Add support for tracking flagged nodes

My first inclination is to just use views for this. Was this discussed and decided against?

Awesome, thanks for the quick reply.

The first item being the user/N/track/subscriptions and tracker/UID/subscriptions callbacks, when we go to render the page, we need to know what flag we're operating upon. So - more questions:

1) has the name of this flag been determined?
2) is this flag already present and is it in the database or is it exported to a particular module?
3) if not, are we prepared to either a) include that flag here or b) have a new module that simply defines the subscription flag which tracker would then have a dependency upon

The other thing would be to generalize the flag integration and simply have a setting on the tracker2 config page to choose what flag we want to operate upon - which would require an variable_get on our page render.

Thanks for any guidance you can give here.

Ok, I went ahead without the answers to some questions here, but I think this is a good start. Patch simply adds a new form field to the tracker2 configuration which allows you to choose the flag you want to use, then based on that decision, spits out a new 'Subscriptions' column in any tracker page.

I'd like to discuss the merits of this, as it seems perfectly reasonable to me that I could subscribe to an issue from the 'recent posts' list, or really from any list of issues.

By default, flag is checking against the current user who is viewing it - so it always reflects 'my' subscription no matter what list I'm looking at. I don't really think this too confusing if I'm looking at another users tracker and I want to subscribe to something that person is involved in.

This would remove the need for any new menu callbacks I think.

Marking needs review.

Here are some screenshots of what's changed by this patch:

Tracker2 configuration screen:

Any listing provided by Tracker2:

Ah, ok. So then what we're really looking for here is a few things:

1. Flag when _tracker2_add() is called.
2. Unflag when _tracker2_remove() is called.
3. Implement hook_flag() so that when our flag is flagged or unflagged, _tracker2_add() or _tracker2_remove() is called.
4. Make sure this doesn't loop...

The following patch keeps the tracker2 settings configuration to determine what flag we're operating upon, removes the Subscriptions column from the listings that the last patch had added, and adds in an implementation of hook_flag in order to execute _tracker2_add() or _tracker2_remove() based on the 'flag' or 'unflag' event. I placed comments where we should execute the flag within _tracker2_add() and _tracker2_remove(). We should probably use the actions integration for this in the flag_actions.module - but I haven't implemented this just yet - work in progress...

Fleshing out the un/flagging within _tracker2_add() and _tracker2_remove() with a new wrapper function called _tracker2_flag()

This now works two ways:

1) You use the subscribe flag and the node shows up in your list.
2) You comment and the node shows up in your list, also changing the 'Subscribe' flag to 'Unsubscribe'.

One usability aspect to discuss is within the _tracker_remove() where a user keeps their subscription:

    // The user only keeps his or her subscription if both of the following are true:
    // (1) The node exists.
    // (2) The user is either the node author or has commented on the node.

Ideally, even if I've commented on an issue and I hit that 'Unsubscribe' flag - I definitely want to not be subscribed any more.

We should either remove access to this flag based on the criteria we're using here and tell the user so, or make this criteria configurable. That might be another issue however. Please review.

Hrm, helps to attach the patch I suppose...

Code looks perfectly reasonable and implements what dww describes. Since #928110: Expose tracker2 tables and columns to views is done, we are getting close :)

Patch #11 works fine for me.
But I believe once we set Subscription Flag in Tracker2 settings we need to make all old nodes from {tracker2_user} flagged. So users will be able to subscribe and unsubscribe to any node.

+

I was wrong in comment #13 - it seems to not gonna work. I think we need a separate feature that allows to unsubscribe from tracking nodes.
Whatever, patch works fine and the only thing we don't actually need in it is a call of dsm() function :)

There are some nasty trailing whitespaces in the patch but I guess that can be fixed during the commit.

Edit: Oh, I haz something useful to say too!

As mentioned in #1007094: Port Tracker2 to Drupal 7, the improvements from tracker2 have been added to core, so I guess the D7 version of this patch needs to go into flag.module?

+++ sites/all/modules/tracker2/tracker2.moduleundefined
@@ -275,3 +304,23 @@ function tracker2_theme() {
+  dsm(func_get_args());

this looks like debugging code that should be removed before commit, but I don't think it's worth marking needs work just for that one line.

this otherwise looks fine and could be a simple and elegant solution to #34496: [meta] Add Flag module to allow users to subscribe/unsubscribe without posting a comment or is something else missing? It sure looks like all pieces are in place to finally kill +1 comments...

Powered by Dreditor.

Reviewed the patch:

* It works fine and flag integration can be really useful for tracker2 and is really optional.

The patch does in human words:

* "Whenever a subscription is added or deleted the corresponding flag is synchronized."
* "Whenever the flag is set to unset one a node the tracker subscription is added or removed."

Re-roll of patch with git against latest 6.x-1.x branch with debug code and trailing white space removed.

Supplied also an interdiff patch to show differences.

Now we just need this to be commited by dww or David Strauss.

Edit: Found a problem:

* The _tracker2_flag function is ignoring the $uid parameter and always flagging as the current user. While this may be valid today, it might not be valid when the code changes.

Marked as "needs work".

Best Wishes,

Fabian

New patch based on the review from #18 with $flag->flag using $account for $uid parameter.

Please review again.

Best Wishes,

Fabian

There is some nitpicky stuff like comments not having full stops.

Also

 $changed = db_result(db_query('SELECT changed FROM {node} WHERE nid = %d', $content_id));

This should use node_load() - there is a 99% chance of the node being loaded during the same request that this happens - flag itself uses node_load() (via $flag->fetch_content($content_id)), so it can be pulled from the static cache instead of the direct database query.

Fixed catch's comments from #20.

Looks good to me now.

ouch.

I'm working on #397458: Revamp mailing logic to leverage flag module, and @amateescu just pointed me to this issue.

project_issue doesn't have a dependency on tracker2, so this "subscribe" flag cannot be re-used for issue subscriptions in terms of e-mail notifications... :(

On a second though, the setting doesn't necessarily have to be "re-used". It's perfectly fine to have two separate configuration settings for PI and tracker2 on which flags to use -- the use-cases are actually different, too, so in a non-d.o scenario, someone could validly configure two different flags for issue views and e-mail subscriptions.

Would these kind of features trigger the need for a contributed module version for D7?

sub

Has anyone reviewed this to verify that the recursive flagging doesn't go into a loop?

Also, I'm not sure this is fully getting the point of being able to flag content in order to make it show up in your profile. That's the only time that we should be acting here, and there doesn't seem to be any reason to flag content that the user is subscribed to unless they explicitly flagged it.

To that end, I think all we need to do is mark the user as subscribed when they flag a node, we don't ever need to flag a node for the user on their behalf.

Removing _tracker_2_flag() and both the calls to that, should do the trick.

Talked with sun in IRC about this, and was informed that the logic is there to auto subscribe users who comment on a node. I'm going to test this out, but I'd rather see that type of logic in project_issue or even in drupalorg.

Based on what I'm seeing at https://drupal.org/node/397464 we don't need the code that flags the node if the user subscribes. This is configurable with Actions.

mmm, not sure about @mikey_p's remarks...

1. First of all, I think we need to stop using the term "subscribe" in comments of these issues/discussions, as it can mean completely different things depending on context/module:
   • project_issue "subscribes" a user to get an e-mail notification upon commenting on an issue. About to be changed/replaced with (optional) Flag integration in #397458: Revamp mailing logic to leverage flag module
   • tracker(2) "subscribes" a user to a node upon commenting on it (or creating the node). About to be changed/replaced with (optional) Flag integration in this issue.
   • There will ultimately be a "Subscribe" flag that allows users to keep track of an issue/node (in one or both aforementioned ways), without leaving a comment on it.

   AFAICS, @mikey_p meant "user commented on an issue" wherever he used the term "subscribe".

2. Has anyone reviewed this to verify that the recursive flagging doesn't go into a loop?

   Sounds like something that's much more simple to verify in a unit test...

3. The automated flagging of the node upon posting a comment was indeed discussed and desired from the beginning. We don't want to entirely lose the current behavior, but instead, mainly remove noise:
   • If you are only interested in an issue/node, but you don't have anything to contribute, then you only flag it.
   • If you do have something to contribute, then you will comment on the issue/node, and if you contribute to it, then you're most likely interested too, so automatically flagging it saves contributors from having to perform two separate actions to stay in the loop.
4. Regarding #397464: Action for executing a flag -- Is Trigger module enabled on d.o, or is the intention to enable it on d.o? I.e., without Trigger, we cannot setup the flag action to perform when a user comments on a node.

   Right now, it looks like all patches for this initiative are based on the assumption that Trigger/Actions is not available, and thus, they are manually adding corresponding integration behavior to the respective modules.

   I mean, in other words, this entire patch is kinda pointless (and actually, most of tracker2's node/comment module integration code) if you base the entire functionality on actions. Instead, you'd implement actions for tracker2, and configure them for the desired "when a comment is posted" and "when a node is created" triggers...

   However, I don't have the impression that basing everything on Trigger/Actions really was the intention.

   Lastly, also note that Trigger module will most likely be removed from D8 core...

freshening up this patch a bit:

1. changed the 'subscribe' terminology to 'follow', which we're using in project issue for this same feature based on UX feedback.
2. added a helper function for getting the follow flag, which also wraps in the module_exists() check.
3. got rid of the helper function for flagging/unflagging, it seemed unnecessary, especially since flag already has checks internally to see if an issue is currently flagged/unflagged before it does anything.

little bit cleaner implementation of flagging/unflagging, flag module already provides a higher level function where we can just pass the flag name.

ok, looking better now:

1. check the node types that the follow flag supports
2. check to make sure that the user hasn't already flagged the node.
3. tear out the automatic unflagging of nodes. the idea is to automatically add a flag when adding something to a node (because if you added content to a node then you most likely will want to follow it) -- once you're following something then unfollowing should be opt-out, not automatic. the only other case to worry about is the node being deleted, and flag module handles that cleanup already.
4. tear out the automatic untracking when you unflag a node. the idea is to automatically add tracker data when you flag a node (you 'did something' with the node, so it should be tracked). once you've flagged a node it makes sense that it should live on in tracker data.

the one weirdness left is that _tracker2_add() gets called twice, because when the flag is added in _tracker2_add(), it calls hook_flag(), which calls _tracker2_add() again. there's no infinite loop, because once a flag is added, the is_flagged check prevents a re-flagging attempt. still it's an unfortunate inefficiency. at this moment, i can't think of a clean way to work around this problem -- we could add a _tracker2_add_and_flag() wrapper function, which calls _tracker2_add(), then use _tracker2_add_and_flag() everywhere except tracker2_flag(), but this doesn't seem like a very robust abstraction to me.

Just reading patches to gain better understanding...

I noted that the docblock for tracker2_get_follow_flag() has the wrong indentation. Also my understanding is that the first line of a docblock should start with an active verb (although that may only apply to D7+ code).

docblock should start with an active verb

Specifically, it should start with a third-person singular present indicative transitive verb.

1. dww and i decided that adding an argument to _tracker2_add() would be the cleanest way to solve the problem of the function getting called twice. the arg defaults to enabling auto-flagging, and we simply set it to FALSE when we call the function in tracker2_flag().
2. added back untracking when a user unflags
3. fixed the docblock for tracker2_get_follow_flag()
4. we decided that it was best to not unflag content when for some reason it's untracked. flag module will clean up when a node is deleted entirely, and the user can opt-out in other cases.

not tested.

Not sure if it is appropriate here or should be opened up in a follow-up issue... However, I thought I should raise it since there is a docblock for tracker2_flag(). According to Documenting hook implementations, the first line of a hook implementation should be "Implements hook_". At minimum, the new docblock should follow this pattern.

This is a change from the former "Implementation of hook_" pattern that was in effect until recently. The old standard is used elsewhere in these files and need to be changed at some point. How should the other docblocks be changed? Here or through a new issue?

@Lars Toomre: There are things that really shouldn't matter at all for major patches like this. At least in contrib, that is. ;)

Sorry if my comment #41 was inappropriate... I thought that the intent was to get the code that drupal.org relies upon very close to core standards to help ease maintainability. If variations from documentation standards are fine, so be it. I not trying to hinder the implementation of this important flagging functionality.

@Lars Toomre:

I thought that the intent was to get the code that drupal.org relies upon very close to core standards.

Some related discussion.

attached addresses all of the points in #40

Updating issue summary to reflect the real plan as it now stands.

Latest patch looks OK to me.

I'd like to test this, but I'm not sure how, since it doesn't look like the data migration is done yet on the test site.

http://subscribe-drupal.redesign.devdrupal.org/node/1236280 is an example of an issue I've created/commented on, but the flag in the upper-right says "Follow" instead of "Unfollow" as I would expect.

However, the patch looks sane. The main thing I would think would break here is if there were assumptions made about all flags affecting the viability in the tracker2 tables, but there's an explicit configuration page for this. This is good, since we might at some point want to have a "Add this project to your favourites" flag, and we wouldn't want that to show up in the Tracker.

+++ b/tracker2.module
@@ -324,6 +336,48 @@ function tracker2_theme() {
+  $flag = FALSE;
...
+    $flag = variable_get('tracker2_follow_flag', FALSE);

It would be safer to use the same data type for the default/false value; i.e., an empty string ('').

Alternatively, use a type-agnostic comparison in code elsewhere to check and compare the flag name.