Allow Views2 listing of users who have flagged content

Thanks.

Could you please give an example, or two, for a query, or task, that would become possible with this patch?

(Incidentally, you're using a buggy version of the module. The Views support has been fixed two days ago. However, its general structure hasn't changed, so you don't need to worry about updating your patch. Don't bother re-rolling it, I'll do it myself.)

I beat you to the re-roll mooffie :)

Here's a re-roll for the latest version. This patch enables the much fabled "create a list of users who have flagged content". Specifically, the relationship it adds makes it so when you make a view that is a list of users, you can get the flag information (such as when they flagged the piece of content). The second part adds an argument, so that you can make a view that takes the content ID as the argument and filters down the list of users to include only those that have flagged that piece of content.

In this re-roll, I cleaned up terminology and the names of things (the default label is now "flag user" instead of "flag", the relationship is "flag_handler_relationship_user" instead of "flag_handler_relationship_content_user").

One thing that *doesn't work*: trying to add the Flag Ops field to a list of users. I imagine this should work by only displaying the flag link for the current user (if the user is in that list). Right now the node/comments table isn't resolving correctly and it causes a SQL error.

Screenshot also attached of desired functionality, walkah sent to me earlier.

I already gave it my best shot trying to figure out how to make the Flag Ops field work in a list of users, so if you get a chance to try getting it working, I'd appreciate it.

Something else that'll need to be addressed: The content ID will need to be resolved to the node or comment title when the argument is used in the page or block title. Instead of the title reading "User's who have flagged [node-title]", it's just doing "User's who have flagged [content-id]".

Thanks for the explanation (and the screenshots).

I understand the details.

But I'm afraid I don't understand "the big picture".

For example, why not simply have a view of type 'node', pull in the 'flag: user' relationship, and voila?

Oh, good question. Walkah?

We'd at least still need the content_id argument though.

I'd think this would be a good thing, simply because if a user wants to make a list of users, it seems natural to select "Users" when creating a new View.

Also, using Users as a base table generates a more efficient query.
Through node table:

SELECT node.nid AS nid,
   users_flag_content.name AS users_flag_content_name,
   users_flag_content.uid AS users_flag_content_uid
 FROM node node 
 INNER JOIN flag_content flag_content_node ON node.nid = flag_content_node.content_id AND flag_content_node.fid = 2
 INNER JOIN users users_flag_content ON flag_content_node.uid = users_flag_content.uid
 WHERE flag_content_node.content_id = 143

Direct to user table:

SELECT users.uid AS uid,
   users.name AS users_name
 FROM users users 
 INNER JOIN flag_content flag_content_users ON users.uid = flag_content_users.uid AND (flag_content_users.fid = 2 AND flag_content_users.content_type = 'node')
 WHERE flag_content_users.content_id = 143

BTW, my original comment was much, much longer. I was sitting here trying to figure out things. When I read your first comments I thought "but why would anybody want to list all users who have ever bookmarked anything?" Having a user-type view as a starting point seemed puzzling to me because I didn't know limiting it to a specific content-id was what you really had in mind. So, why not just start with that content-id? (read: "a node-type view").

One thing that *doesn't work*: trying to add the Flag Ops field to a list of users. I imagine this [...]

But suppose it _did_ work, what purpose would this Ops field serve here? I think an Ops field is only useful when displayed alongside node(s) (or comment(s)), isn't it? If it's displayed alongside a user, it gives the false impression it's used to subscribe/unsubcribe a user to this node, but our module supports only subscribing the _current_ user.

===

Here's a "who" view I hastily made:

http://pastebin.com/m3707560e

(It supports everything in your screenshot, no hacks were used, and I think the structure is straightforward: have a 'Node ID' argument to focus on your content, pick the 'flag' things, pull in the 'flag user', and that's really all.)

Also, using Users as a base table generates a more efficient query.

Not so. You want to show a specific content-id. So you use the 'Node: ID' argument, and you end up with:

SELECT node.nid AS nid,
  ...
WHERE node.nid = 123

Quite efficient, I think.

I'd think this would be a good thing, simply because if a user wants to make a list of users, it seems natural to select "Users" when creating a new View.

In my previous comment (I wrote it before I saw your new comment) I explained why I actually didn't see it as something intuitive. Some more thought is needed here. But I'm open to changing my mind. I opened an issue "Rework Views2 support", you know. I'll be thinking about this.

how can we get this functionality into flag?

Note that the functionality of "listing users who have flagged a piece of content" has always been there:

Start with that piece of content, and pull in the Flag's user relationship. That's all.

(BTW, I'm all for providing a "default view" for this.)

To me, it makes sense that if I am "creating a listing of users" that it be a User view type, since I'm creating a list of users.. and the conditions are filtering based on certain flags.

First,

When you start a user-type view, it doesn't necessarily mean you want to generate a list of users who flagged something. For example, since this module will support flagging users, starting a user-type view might mean you want to generate a list of users who *are* flagged.

Whatever,

The Views support of this module certainly needs improvenets/rework. There's no reason why your way of thinking shouldn't be incorporated. I've started a stub issue for this already. It's just that "First Things First": There's one cleanup patch awaiting Nate's approval, and there are more cleanups on the way, and they'll make things easier (I have great motivation, but my work is stalled becuase I need to wait days/weeks for approval). I'm against touching the Views support till the code is cleaned up, because then we'll have to waste our time working double on the upgrade paths instead of doing real programming.

re: flag ops - is that flag/unflag? I don't think it makes sense in a user-based view.

Ops doesn't make sense in that view, sure.

(But it'd make sense when it's *users* that we're flagging.)

can it not be omitted?

Just don't put it on your view.

how can we get this functionality into flag?

One good way to move things forward is to review patches so that development can go forward. For example:

(Views) #288599: 'skip base' doesn't work.
(We're likely to need this 'skip base' feature, to make our Views support intuitive.)

(Flag) #291042: Code cleanup

I still think this is ultimately necessary. Choosing a "user" type of view to list users just makes the most sense. As I said in #7, the resulting query is also more efficient (a single JOIN instead of two JOINS).

I wholeheartedly agree that it's necessary to improve the Views support. In fact, I hate it as it is.

I hope, this week, to finish with some documentation tasks, some little bugs, release a new beta, and then work on the Views support.

I have an elegant solution almost ready. I'll post it later for review.

In short: it involves making the 'flag_content' table a 'base' table.

(Nate, could you please approve #297739: Views maintenance patch?)

Here's a reroll, correcting the raw content ID from being displayed as the image shows from #4, thanks to our new get_views_info() method.

The patch hasn't changed much, even with the Views cleanup. Because the content ID is in one of our own tables, there's no need to loop through the different types to support it as an argument.

My original Views implementation (which I never posted) included the flag_content table as a base table, but ultimately I decided it wasn't the logical thing to make a base table (since you can already make lists of the same content by selecting the appropriate user/node/comment base table).

From my own experience, when Views asks me the "View type", fundamentally I expect that to be "the thing I want a list of". Adding flag_content as a base table won't help this because I want a list of "users", so I select "User" as the view type. Starting with "flag_content" as a base table to get a list of "users" who flagged something (not users that have been flagged), doesn't make any more sense than starting with the node table. I think this is a general mentality: the View type corresponds to what the listing contains.

I'm fine with the patch! (you can commit it after you tend to the following.)

Two things:

+  // Add a relationship for the user that flagged any type of content.
+  $data['users']['flag_user_rel'] = array(
+    'group' => t('Flags'),
+    'title' => t('User'),
+    'help' => t('The user that flagged an item.'),
...
+    $options['label'] = t('flag user');

Admins are going to see "Flags: User: The user that flagged an item". There are two problems here:
1. This isn't true: We aren't linking to a user, but to the _flaggings_ of a user.
2. There's already a relationship with the same title/help. It's going to confuse admins.

I suggest we change it to:

+  $data['users']['flag_flaggings_rel'] = array(
+    'group' => t('Flags'),
+    'title' => t('Flagging'),
+    'help' => t('Get at the individual flaggings of a user.'),
...
+    $options['label'] = t('flaggings');

The following isn't needed:

+    $join->definition['extra'] = array(
...
+      array(
+        'field' => 'content_type',
+        'alias' => $this->alias,
+        'value' => 'node',
+        'numeric' => FALSE,
+      ),

My original Views implementation (which I never posted) included the flag_content table as a base table, but ultimately I decided it wasn't the logical thing

(I'll leave my proposal to a different issue. The more I think of it the more I see its beauty. It's not exactly flag_content that I turned into a base but an alias of it I nicknamed "flaggings".)

(If you're bussy and want me to commit this patch, let me know.)

From a database point of view, what you propose is correct. However, from a human point of view, I don't want a list of "Flaggings", I want a list of *users* who happen to have flagged things.

IMO we should meet the expectations of the human, who does not have underlying knowledge of the database schema.

Alright, here we go.... after some pondering I came to realization that we already start with the User information (since it's the base table), and therefor it doesn't make sense to call the relationship "Flag: User".

The term "Flagging" might be more technically accurate, but it introduces new terminology without describing what it accomplishes. This patch instead names the relationship what it is: "Flagged content". We start with the user information, then we get information about content that has been flagged.

(@webchick: I agree that things should be straightforward to users.)

Nate,

Your 'title' and 'help' seem fine. I don't have a better idea right now.

We can change them later.

However, one important string to change right now is 'flag_user_rel' because we won't be able to change it easily later. 'flag_user_content_rel' looks like a fine choice.

I'm attaching the same patch but with these mainly cosmetic changes:

1. 'flag_user_rel' -> 'flag_user_content_rel'
2. t('Flagged content') -> t('User flagged content')
3. t('flag') -> t('user flagged content')
4. class flag_handler_relationship_user -> class flag_handler_relationship_user_content
5. I removed the "Leave unchecked if needing to [...]" from the 'required' description (it's incorrect).
6. "who've" -> "who have"

In flag.inc, I changed Comment's title field to 'subject'.

(I'm not commiting this myself because you may be modifying flag.inc now, for the Actions support, and I don't want to invalidate your patch.)

Here's a more detailed response to webchick's #19, about "flagging" entities not being a very good idea.

But this is only a hypothetical discussion: I'm fine with the patch. (I know you're busy, no need to reply.)

However, from a human point of view, I don't want a list of "Flaggings", I want a list of *users* who [...]

You don't have to have a view of type "flaggings" just because it's possible to do so. Views of type 'users' are fine. But note that:

1. You do use these "flaggings" nevertheless: That's effectively what Nate's patch does.
2. "Flaggings" do exist in the real world: as push-pins, dog-ears in a book, etc. You do look for the dog-ears, and not scan every page, when you open a book. The two approaches --starting with flaggings vs starting with content-- are two different ways to look at a problem, and the user could have a choice.

from a human point of view [...]
I want a list of users who happen to have flagged things

...flagged things with a certain Content ID.

Yes.

But what about flagged things of type "recipe" and tagged as "vegetarian"?

Humans expect this too. In fact, they are more likely to expect this.

The patch does not allow this.

IMO we should meet the expectations of the human, who does not have underlying knowledge of the database schema.

Our avoidance of the term "flagging" hasn't helped us in freeing the user from the need for knowledge. I'm afraid it's the opposite. Look at the patch we're going to commit:

Title: "Flagged content"
Help: "Limit results to users that have flagged certain content."

These two labels can only be understood by a user with database knowledge (1. The relationship doesn't actually bring in the "flagged content"; 2. The help text alludes to sql's inner join.)

Here's a re-roll.

- The relationship handler was simplified as explained in #302970: Simplify the relationship handlers
- "User flagged content" -> "User's flagged content"

However, I'm temporarily marking this "postponed". I don't think we should include this in the upcomming "beta4". We can commit this to the "dev" that comes after, so that we have time to change our minds.

I forgot to attach the re-roll.

Here's a re-roll.

A reminder to whoever commits this eventually: do cvs add includes/flag_handler_argument_content_id.inc.

Please ignore the previous patch. It contains a typo. Here's the correct patch.

This all looks great and applies to the current version without problems. I think we're ready to put this in.

After going back and forth, I needed put this in for the publication of our book which includes this functionality.

Is this committed to version beta6, or do I have to use DEV to get this functionality?

**EDIT: Nevermind, i figured it out:)