Blocked users should not be included in flag_counts

The concept makes sense for this patch, though I wonder if we should extend this to nodes and comments as well. If blocked users aren't listed, then unpublished nodes or comments wouldn't be listed either. However I think unpublished nodes are regularly flagged, so it's possible that a similar situation exists for blocked users. In fact you could set up an action to block a user when it is flagged if you wanted. So maybe this isn't something we'd want to get into.

If we did implement this, I think there's a bug in the current patch.

INNER JOIN {users} u ON fc.uid = u.uid

This should probably be:

INNER JOIN {users} u ON fc.content_id = u.uid

Otherwise the query makes it so that requesting a count for a blocked user will always return 0, rather than excluding blocked users from the count.

Reopening this, as I

• Don't understand sirkitree's point about "flag_friend implementing something to take care of this"
• Don't understand why this would be controversial

I have no idea how flag_friend would 'take care of this'. Would it remove the flag? But what if that user becomes unblocked? I believe that this should be handled by Flag. Heres my battle plan

_update_count() and inner join the users table like so

$count = db_result(db_query("SELECT COUNT(fc.fcid) FROM {flag_content} fc INNER JOIN {users} u ON fc.uid = u.uid AND u.status >= 0 WHERE fid = %d AND content_id = %d", $this->fid, $content_id));

That fixes the get_count() method. So then on the hook_user, case 'update' and status changed (0 /1), get all content_id / fid that the user has flagged and have that recounted.

Then you have this patch, and its changes to the get_user_count() on the user flag

ok so heres the patch.

So to further this discussion, here is what happens with this patch.

When a user's status changes, all of their flagged content is recounted in the flag_count table. The value that is inserted in into the flag_count table takes into account whether or not the user is blocked (status = 0). So whenever you call $flag->get_count($content_id) you will receive the number of unblocked users who have flagged the content_id.

For user flags, when you call $flag->get_user_count($uid) you will get the number of unblocked users the $uid has flagged.

Nodes and comments

I do believe that the $flag->get_user_count($uid) should return the number of published nodes/comments that the user has flagged. But that might be debatable.

However useful this is, I think it somewhat constitutes a basic change to the API

There is no API change here.

I don't view it as a feature request, it is erroneously counting blocked users as valid flaggers. If a user flags a node, then gets blocked. That flag shouldn't be counted, but shouldn't be discarded either in the event the user is unblocked. This issue isn't "Boy it would be glad if Flag did X", its "Boy thats wrong....". Therefore, not a feature request...

It doesn't look to me like get_user_count() serves any purpose, can we remove it?

We also should try to find a better mechanism than a static variable in hook_user() for re-calculating flag votes. This mechanism seems like it might cause problems when doing batch changes to user accounts at admin/user/user.

I have actually used get_user_count() to determine how many pending flag_friend requests a given user has.

We also should try to find a better mechanism than a static variable in hook_user() for re-calculating flag votes. This mechanism seems like it might cause problems when doing batch changes to user accounts at admin/user/user.

Ahh good point. So it should probably be an array keyed by uid? I think that would work. Any other ideas?

I have actually used get_user_count() to determine how many pending flag_friend requests a given user has.

That sounds like what the flag_get_user_flags() function is for (even though it doesn't have an option to pull out a single flag), which caches the user's flags per request, so you don't end up doing a query for every flag.

So it should probably be an array keyed by uid?

That would make me feel better about it and I think it would work.

That sounds like what the flag_get_user_flags() function is for (even though it doesn't have an option to pull out a single flag), which caches the user's flags per request, so you don't end up doing a query for every flag.

The problem I have with that, is it can't ever take into account special cases for a flag. flag_get_user_flags() would have to join in the specific entity table (node, comment, user) to determine if the entity is published/blocked.

That function has the advantage that for N flags, it does exactly one query, but each flag can't have different rules for counting (as pointed out above).

Anyway this is an incremental patch, doesn't take into account adjusting flag_get_user_flags(), which does need to be addressed as it offers a performance improvement, but can't handle unpublished/blocked states.

I do believe that the flag object is static cached. So I would like to investigate using the flag object itself as cache for these.

  if (!isset($this->user_counts[$uid]) {
      $this->user_counts[$uid] = // QUERY HERE
  }
  return $this->user_counts[$uid];

Then flag_get_user_counts() could fetch all the flags and ask each one. Then you have things static cached, and respecting published/blocked states. But would cost a query per flag, uid, (and maybe content_id) whereas current implementation would cost a query per uid (and maybe content_id).

Thoughts on my ramblings?

The problem I have with that, is it can't ever take into account special cases for a flag. flag_get_user_flags() would have to join in the specific entity table (node, comment, user) to determine if the entity is published/blocked.

Sorry I had the wrong function, $flag->get_user_count() is the same as flag_get_flag_counts() is what I meant.

You sure?

/**
 * Get flag counts for all flags on a node.
 *
 * @param $content_type
 *   The content type (usually 'node').
 * @param $content_id
 *   The content ID (usually the node ID).
 * @param $reset
 *   Reset the internal cache and execute the SQL query another time.
 *
 * @return $flags
 *   An array of the structure [name] => [number of flags].
 */
function flag_get_counts($content_type, $content_id, $reset = FALSE) {

 /**
   * Returns the number of items a user has flagged.
   *
   * For global flags, pass '0' as the user ID.
   */
  function get_user_count($uid) {

/me head explodes...

Oh geez. ANYWAY what I'm saying is I am fairly sure we already have an API function that does the same as get_user_count(). We've specifically moved API functions for data retrieval outside of the flag objects because each instance of a Flag would have separate static caching, so it doesn't work well in those situations. So basically, let's look at our existing API functions, see which one lines up and make the change there rather than adding a new one.

Hmm, okay I *can't* find an exact match. However I'm not keen on moving all our API functions into Flag methods. There's no reason why some CRUD functions are in the object while others are functions. I'd prefer to keep them separated from the objects so that they can be more readily accessed by Themers or user's new to the APIs, and allowing the use of a flag name instead of needing to load a flag via flag_get_flag() is one less line for common calls.

So what I'd like to see is a couple of things:
- Ditch the get_user_counts() method and make flag_get_user_counts() a function instead.
- Update for the 2.x version, this will not be added to the 1.x version.
- Update flag_get_flag_counts() to accommodate for this change, and then use this API function in hook_user() instead of doing a manual query to get the count.
- Accommodate for the Session IDs in the 2.x version (in addition to UIDs)

Ditch the get_user_counts() method and make flag_get_user_counts() a function instead.

Don't see that happening without building yet another (tm) query builder. Im *highly* skeptical. flag_get_user_counts() will have to do a switch on flag type (node, comment, user) to determine what table holds the 'state' (published / blocked). And by doing that, you have now removed the abstraction provided to flag objects.

I do think that flag_get_user_counts() could be a wrapper around $flag->get_user_counts(). And that would appease your themer concern but would add a little more load on the db.

I don't see why joining onto a table has anything to do with it. Just take the query from $flag->get_user_counts() and put it into flag_get_user_counts().

OH. Oh. Ohhhhhhhhh.

I've been reading the $flag->get_user_count() function wrong the whole time. I thought you were excluding the users from the count when asking "how many users have flagged this content". But what it actually does is exclude the users from "what users have I flagged". Oh sheesh. Well that all makes a lot more sense. Since you can't "block" nodes or comments, it doesn't really make sense to have that option.

I think what got me all confused is that these two changes are completely separate, two different problems. They're just related in that they both have to do with blocked users. In that case, I think we're pretty much ready to go, other than hook_user() should be using an updated flag_get_flag_counts() function instead of doing a manual SQL query to get the count.

Ok now that I got you on board, one thing to decide on as well.

Since you can't "block" nodes or comments, it doesn't really make sense to have that option.

While that is literally true, you still need to handle unpublished nodes or comments

Back to #5

I do believe that the $flag->get_user_count($uid) should return the number of published nodes/comments that the user has flagged. But that might be debatable.

Essentially, each flag type would override get_user_count($uid) and JOIN there tables to check the 'state'. So flag_node would JOIN node on nid = content_id AND status >= 1, and flag_comment would JOIN comment ON cid = content_id AND status = 0 /* wow really, thats what comment_schema says */;

wow comment status is 0 = published in 6. The opposite in 7.

http://api.drupal.org/api/function/comment_update_7001/7

thats a wtf :-D

Does this patch need a re-roll or is it good as done in #11?

Well we definitely need work on this issue now that we've switched to an increment/decrement system in #719616: Keep flag_counts current by manual in/decreasing of count. . Unfortunately this makes this issue significantly more difficult to solve because counts in flag_count are not directly tied to the number of rows in flag_content. We would have to go through an manually unflag all their content. But then if they were re-allowed onto the site, we'd have to re-flag all their content. Imagine if you were accidentally banned and then you lost your list of bookmarks when your account was re-activated.

Needs fixing on 3.x first and backporting.

> We would have to go through an manually unflag all their content. But then if they were re-allowed onto the site, we'd have to re-flag all their content. Imagine if you were accidentally banned and then you lost your list of bookmarks when your account was re-activated.

Though I'm rather tempted to say that given the complexity of doing this, this should be left to a contrib module.

Drupal 7 is EOL. Issue will be closed, but patches are still here