Provide a data migration path for following issues via flag.module

Forgot to tag

@Lars: Thanks for caring about this and sharing your thoughts, but I don't really understand your concerns:

- Flags defined programmatically can still be overridden in the flag admin UI.

- I don't think project_issue isn't going to hard-code that only its flag can be used for this feature -- we're still going to let you use a different flag if you really want to. We're just going to provide a default that's going to work out of the box.

- If anything, having the flags we need defined in code will speed up deployment, since we don't want d.o deployment processes that look like "enable flag.module, go to the admin UI and click together a flag for this, ...". The more these settings are captured in code, in version control, etc, the easier and more reliably we can deploy them.

The tracker2 thing was just meant to be a possible d.o optimization. Either way, we definitely need the other case working.

Furthermore, after pondering a bit and chatting with killes, I think we should do this data migration while d.o is still live -- since we're using tracker2 for all the issue queue views and the tracker page, the main things that would be broken without the flag data are e-mail notifications and the state of the "Follow" vs. "Following" UI on the issue pages for each user (all the work at #1284694: Tweak UI of issue following (bluecheese) -- much of which I'm planning to fold into project_issue itself). We can wait to deploy the new follow UI until the data migration is completely done. And we could even leave the old e-mail logic in place to send emails (basically temporarily hard-code it to ignore the follow flag setting and pretend we're still in the no-flag case for those hunks of code in mail.inc -- another good reason to leave flag support optional in there).

Therefore, please carry on getting real data and ignoring tracker2.

Thanks!
-Derek

FYI:

- I created a new feature branch for flag integration: "34496-flag-integration"
- I finished up #397458: Revamp mailing logic to leverage flag module and merged the old feature branch into that, then deleted the old branch
- I just pushed a commit into the 34496-flag-integration branch for programmatically defining the default flag:
http://drupal.org/commitlog/commit/1894/387b07f0ff0cb830702180ac00ff9485...

Hopefully that'll help while you're working on the data migration stuff.

Thanks,
-Derek

Thanks for the great summary! Feel free to put that in the issue body.

I'm strongly in favor of #2, and I know killes and nnewton will prefer it too. Quicksketch was also in agreement.

Cheers,
-Derek

I haven't tested, but a quick visual inspection makes me wonder:

A) Can't $flag->type be an array of node types? If we're hard-coding 'project_issue' in the query against {node} why not just use $node->type or continue to hard-code 'project_issue'?

B) Why separate queries to insert each node flag? why not a single query to do all the issue flags in one pass? Something like this:
db_query("INSERT INTO {flag_content} (fid, content_type, content_id, uid, sid, timestamp) SELECT %d, type, nid, uid, 0, created FROM {node} WHERE type = 'project_issue' AND status = 1", $follow_flag);
?

Sure, we still have to handle the counts separately, but this seems like it's going to save a ton of separate queries.

C) For that matter, why not the same with comments? Can't we do something similar to the above? I guess the big problem is about avoiding duplicates? Can't we let the DB sort that out instead of doing it in PHP? Maybe it doesn't matter.

D) What about new content created between when this batch update is initialized and when it completes (assuming we're not going to take d.o offline while this runs)? Aren't we going to miss those? Or is the idea that we deploy the new code that's auto-following new content so that new flags might be getting inserted while this is all happening?

Thanks,
-Derek

A) Duly noted, thanks.

B) Yup.

C) Okay, fair enough. So we're going to have a bazillion queries for this update. At least it's not 5 or 10 bazillion. ;)

D) I think it's not going to fly to take d.o offline for potentially a few hours, prohibiting 1000s of people from getting things done, just so we don't have to handle this race condition ourselves by writing this update in a way that can handle some duplicates. That's going to be a hard sell to the rest of the infra team, and for good reason. I believe #8 spells out a pretty sane plan for deploying that will a) keep the site live the whole time and b) prevent anyone from seeing weirdness while things are in an inconsistent state. The only trick is to make sure that we actually migrate *all* the data.

One way to do that is to basically deploy the tracker2 change first so that we start populating the flags with new content, *then* go back and migrate everything -- we just can't assume no duplicates. It's not even obvious to me (I haven't investigated closely) why we need to worry about duplicates ourselves -- can't we just let the DB prevent those? {flag_content} already has a unique key defined on array('fid', 'content_id', 'uid', 'sid') which seems to be exactly what we want. I guess that'd be a reason not to try to do all the node flags in a single query -- if we hit a duplicate key error, better for a single flag insert to fail than the entire query.

That said, the stakes are not *that* high if we miss a few. Honestly, I'd much rather announce "On Day X while this is being deployed, if you create new issues or comment on existing ones, you might not automatically be following them and you should check the fancy new button to make sure you're following everything you care about." That'd be much better than saying "On Day X, no one can use drupal.org at all for N hours, period." Of course, I'd rather we just got this working in a way that can handle new content as it's coming in so we don't have to announce either. ;)

Thanks,
Derek

Re: readonly -- that's still a pretty big burden to put on the Drupal community just to handle a relatively minor edge case. But, it's certainly better than taking the site offline. Either way, I'm definitely okay disabling changes to issue notification settings -- that's not a big deal at all.

Another thought I had was something more like this for the deployment order:

1. Enable flag.
2. Deploy the project_issue code, but disable the auto-flagging, hide all of the new UI (and disable editing per-project issue notification settings), and tell the e-mail notification code to keep using node/comment authorship to decide who gets notified. In case it's not obvious, I mean deploy the new code with all that stuff pre-disabled. ;)
3. Run the main update (could be hours). We can use the single query for flagging all nodes since there's no chance of duplicates. At the end of the update, have it store somewhere (e.g a variable_set()) the last nid and cid it processed.
4. As atomically as possible, record the max cid and nid on d.o and then turn on auto-flagging in project_issue.
5. Run a second update that just looks at the nids and cids between the ones recorded at the end of the main update and the ones we just recorded to handle everything that was created while the main update was running.
6. Turn on the new UI, allow users to manage per-project issue notification settings again, and tell project_issue to start sending notifications based on flag, not node/comment authorship.

In theory, the only race condition here is during step 4 between the instant we record the current max nid/cid in and enabling the auto-flagging, which should be a very small fraction of a second, and I'm definitely not worried about that. The main update in step 3 can still be written "naively" to assume {flag_content} is empty (at least insofar as our follow flag is concerned) and doesn't have to concern itself with handling duplicates. In fact the 2nd update doesn't need to worry about duplicates either since we're going to have the range of nids/cids to handle (although it probably wouldn't hurt to be a little more resilient to duplicates in that one).

Other than slightly more work for us to get all this happening properly, I don't see any problems with this approach. Do you? d.o stays up (and read/write) the whole time. There's no visible weirdness or inconsistent states. It should migrate all the data, without making things overly complicated when writing these upgrade paths. Thoughts?

---

Re: existing content in tracker2 -- I don't understand your question. All the existing content in tracker2 is also in {node} and {project_issue_comments}, which is what we're worrying about here. Following forum posts is out of scope for the time being (although should be relatively easy to do in a follow-up issue). What are you worried about in tracker2 that's not already covered?

Thanks,
-Derek

Excellent, very excited to be converging on a viable plan here. Setting this back to needs work for the following:

- Increase the batch size from 100 to 1000
- Add something to the end of that update to variable_set() the last nid and cid processed in the batches
- Write something to handle the 2nd update pass that's restricted to the range of nids/cids as per #17
- Put in necessary code to make it easy to do the various UI/feature toggling I proposed in #17 (e.g. some checks for killswitch variables or something)
- Write something to do the "atomic" queries and variable set from #17 step 4

I think that's it. I'll coordinate with hunmonk offline to sort out who's doing what. ;)

Wondering out loud if there's a sane way to completely automate all of these steps... I suppose if all the various killswitches are in place, the main update could be project_issue_update_6012 (as it is in the patch), the "atomic" toggle could be project_issue_update_6013 (not a batch), the 2nd pass on the restricted range could be project_issue_update_6014 (batching again just to be safe), and then the final cleanup (e.g. removing the now unneeded nid/cid variables, enabling the UI, etc) could be project_issue_update_6015. Does that seem reasonable?

FYI: #1292746: Refresh staging.d.o with live d.o data for testing flag integration and issue following

p.s. @moshe: That's not going to fly. Way too confusing and lots of people would be completely miserable. Do you honestly expect webchick to manually follow every issue in her tracker page?

Totally untested, but this is almost everything we need. There are a few TODO comments remaining. However, it's late and I need to sleep, but I wanted to post what I've got so far in case anyone wants to review. Be sure to pull the latest on the 34496-flag-integration branch before applying this.

Some reasons this is still at "needs work":

A) Instead of littering the code with killswitch hacks, we just want to *not* have 'project_issue_follow_flag' defined until after these updates are done. That simplifies things considerably. You'll notice that project_issue_get_follow_flag() is defaulting to FALSE when it does the variable_get(). So, there's only a single hack in the module code that knows about the updates: the tiny code block in project_issue_flag_issue() (which is well-commented). However, I'm not sure if variable_get('project_issue_follow_flag_for_update', FALSE); should default to FALSE like that, too, or if *that* should default to 'project_issue_follow'. I can argue both sides. I left it as FALSE since I was thinking it'd be safe to have the admin define this in settings.php or via a drush vset before running the update. Then again, if we let that default to our provided flag, it'll all Just Work(tm)

B) The original patch in #12 would have printed a query message about every. single. node. it. migrated. That'd be fun on a site like d.o. ;) Fixed that, although now the main update (6014) doesn't print anything so that's still a TODO.

C) We still need to process all the comments added on the live site during the first pass that aren't attached to nodes created in that pass. I've got the query all set to find them, but I punted on actually flagging them. These are the ones that are most likely to result in duplicate flags, and where we're going to just have to recompute the flag counts.

D) I now realize we should query MAX(cid) FROM {comments} during the initialization for update_6014() and restrict the comment queries to always ignore comments higher than that value during update_6014(). That's the only way to ensure that new comments coming in while update_6014() is running are all handled during update_6016(). Otherwise, there are race conditions that could lead to missing comments (e.g. if a new comments is posted on old nodes that we've already processed, that won't be reflected in the max cid we're recording as we go, but it'll be outside what we query during update_6015().

E) I haven't made update_6016() a batching update yet. I'm not 100% sure we need to. Even if update_6014() takes a few hours to run, there are probably only going to be O(10s) of issues created in that period, and maybe O(100s) of comments. Maybe we should just make it batching anyway, just to be safe, but maybe we can avoid the complication.

Still untested, but this fixes everything from #24 plus a few things hunmonk asked about when we chatted about this patch.

I'm going to do an initial test of this over on http://subscribe-drupal.redesign.devdrupal.org to see how we're doing. ;)

Heh, first attempt at running this was spewing a bazillion errors like so:

Unknown column 'last_updated' in 'field list'      [warning]
query: /* 127.0.0.1 home */ INSERT INTO flag_counts (fid,
content_type, content_id, count, last_updated) VALUES (3,
'node', 1165494, 2, 1306107439) database.mysql.inc:150

That's from the original query from patch #12 to update {flag_counts}.

Let's try again... ;)

And now without whitespace errors...

First real attempt was mostly a success. However, we had a few problems:

- Got some duplicate keys during update_6016() since we had live comments after 6015() ran that had already flagged one of the new nodes created during 6014(). So, we now clear any existing flag records in 6016() before we call _project_issue_update_follow_flag_process_node() -- we also now invoke that helper in a way that it doesn't try to enforce a max cid and just queries for all comments on the node.

- I had reversed the order of singular vs. plural in format_plural() so some of the output messages were bogus. There were also a few other bugs in the messages (e.g. %value vs @value).

- In 6016() when processing the comments on older issues that were posted during 6014, I was using $fid not $flag->fid so the record went into {flag_content} with fid 0.

I also temporarily added a few debugging messages in here just to make sure we get enough data on this next attempt.

Sweet, that almost perfectly worked! ;) Actually, the migration itself worked flawlessly. No duplicate key errors. Everything was flagged as expected. webchick and I both pounded the site while the upgrade ran, so we hit all the expected edge cases:

NOT flagging node 1243552 for user 46549 because of comment 4841371    [success]
(already flagged)
NOT flagging node 1236280 for user 24967 because of comment 4841372    [success]
(already flagged)
Flagging node 1243552 for user 60783 because of comment 4841373        [success]
Flagging node 1238484 for user 24967 because of comment 4841375        [success]
Flagging node 1171828 for user 46549 because of comment 4841377        [success]
Flagging node 1164492 for user 24967 because of comment 4841378        [success]
NOT flagging node 1166476 for user 46549 because of comment 4841379    [success]
(already flagged)
NOT flagging node 1238484 for user 24967 because of comment 4841380    [success]
(already flagged)
NOT flagging node 1164700 for user 24967 because of comment 4841384    [success]
(already flagged)
NOT flagging node 1172214 for user 46549 because of comment 4841387    [success]
(already flagged)
Added follow flags for 4 comments which were posted on existing        [success]
issues while update 6014 was running.

There's only one tiny problem in how one of the messages was printed, but it was a silly bug on my part in the printing, not a bug in the migration.

SO... I'm tentatively calling this a complete success. However, I'd really like to try it on the full d.o dataset on beta.d.o. If only #1292746: Refresh staging.d.o with live d.o data for testing flag integration and issue following were done...

Anyone want to look through the code to see if they spot anything funny?

But, I believe this is RTBC now. HURRAY!

Re: #30: Thanks for the review. However, that last_updated field was the source of errors on my testing. See #26. This column does not exist as of flag 6.x-2.0-beta6 which is what I'm testing against and targeting for deployment. Therefore, I'm planning to go with patch #29.

Although we're still awaiting final testing on a copy of the live d.o database, I've committed and pushed patch #29 to the 34496-flag-integration branch. Calling this fixed for now. We can always re-open if we uncover problems in further testing (which is going to happen once #1292746: Refresh staging.d.o with live d.o data for testing flag integration and issue following is done).

p.s. quicksketch confirmed in IRC that flag 6.x-2.0-beta7 is *not* just around the corner, which is why I'm sticking with patch 29...