Revamp mailing logic to leverage flag module

As you'll quickly find, Trigger is a completely worthless module. You'll need to code everything including the conditional that fires the action and the form for sending an e-mail. There is no way to reuse existing actions or to add configuration to them.

I'm not interested in adding any further integration into Trigger, since it's so entirely inept. I'd suggest using Rules if you really want this to be user-configurable, but if dead-set on trying to make it work with trigger it'll have to be in a separate module.

@sirkitree: Wait - issue status update mails are pretty custom and very closely tied to project_issue. Flag is here to flag the content, but not for providing notification mails.

This one I'm marking won't fix. #397464: Action for executing a flag is what we really need in Flag.

I agree that this is pretty custom, but for the sake of being a pragmatist (at times) and making flag useful for project.module, this may have to be written as a contrib action in rules or in the project module. I don't think it would be difficult, but it would be duplicating the core system email action.

Either way, the mid/long term goal is definitely #329500: support for looping and data lists. According to fago, it isn't within the scope of the impending 1.0 release, but it is already very far along. Here are a few links to help get started:

- one out of the many concept pics: http://drupal.org/files/issues/add_a_loop_dropdown.png
- fago's summary w/ comments: http://drupal.org/node/329500#comment-1126926
- moofie's module: http://cvs.drupal.org/viewvc.py/drupal/contributions/sandbox/mooffie/mis...
-- http://drupal.org/files/issues/list_generators.tar_.gz (for testing)

I'm fine with just making this an action that's provided by project_issue. The issue emails *are* very specific to project_issue, and I don't expect other modules to deal with this. We'd just change the logic right now in all the places that directly fire an email to have conditional flag support. Whether this should become an action that can be triggered depends on how easy it is to make it work with triggers...

this is C from #34496: [meta] Add Flag module to allow users to subscribe/unsubscribe without posting a comment which i'm handling.

now it's E from #34496: [meta] Add Flag module to allow users to subscribe/unsubscribe without posting a comment which i'm handling, plus one other piece. to summarize:

1. We add a new user/N/issue-subscriptions tab for every user, with the following parts:

• A global, site-wide knob for "Email me issues from any project" with choices for "All issues", "Subscribed issues", and "None". We store this *separately* from the per-project stuff. This is a new, per-user table, {project_issue_user_subscription} or something.
• A table of any projects where the user has configured emails on a per-project basis (e.g. http://drupal.org/project/issues/subscribe-mail/drupal). The table shows the current settings for each project, and includes a row with an auto-complete text field to opt-in for emails to other projects. Again, the choices are "All", "Subscribed", and "None". "None" removes that row from the table. Possible feature-creep: a component filter here?
• Other global, site-wide knobs per aclight in #95 (e.g. #219751: Add a master subscription setting to receive all issue email notifications and #15380: Allow to configure content of issue notification e-mails). These also go in {project_issue_user_subscription}. Perhaps we want to add project category and status filters here, too?

FYI: Some older issues related to this which we'll fix at the same time:
#161850: 6 million rows in {project_subscriptions}
#177318: Subscriptions to project issues for new projects added after subscription settings have been saved

tagging

Hi Hummonk,

Any progress here? It looks like this issue is the main blocker to get rid of +1 comments... It looks like a complex issue, maybe it could be broken down in separate steps?

Is there any code to review?

Thanks!

#397464: Action for executing a flag has been commited already a while ago.

Can't we use that one?

Best Wishes,

Fabian

Can someone clarify whether #397464: Action for executing a flag resolves this, or (if not) the specific cases that are not covered?

No, #397464: Action for executing a flag is a totally different part of the problem, which is hopefully clear if you read that issue (especially comments #8-#12). All that buys us is the ability to configure an action so that every time a user comments on an issue, we fire the "subscribe to this node" flag for that user. That doesn't change project_issue's email functionality to be able to send emails to the users who have configured the system that they want email notifications whenever new comments appear in issues they have flagged, which is what this issue is about.

First pass at #6 1)

Most of #6 is extreme feature-creep though. We badly need this on d.o, so any perfectionism and further nice-to-have features really should be deferred to other issues.

Missed a second instance of the obsolete/removed 'project_issue_global_subscribe_page' variable.

Wow, amazing! Thanks sun!!!

I don't suppose you looked at my work over here:
http://drupalcode.org/project/project_issue.git/shortlog/refs/heads/1538...
#15380-34: Allow to configure content of issue notification e-mails

I'm assuming these two patches massively conflict with each other. :( That's highly unfortunate (and what I get for not making time to test and merge my own prior work on this). I didn't realize anyone was actually going to be working on a patch for this...

Anyway, I don't have a chance to review this right now, but if I can, I'll try to take a closer look later (perhaps on my flight home to Oakland this evening).

Attached patch is pretty much complete, also contains the necessary e-mail notification adjustments now.

As mentioned on #404084: Add support for tracking flagged nodes, project_issue uses an own configuration setting to choose the flag name for Flag integration.

Also, I went ahead and implemented the Flag integration as an optional feature. This means that the e-mail notification logic still supports the notion of "own issues"; i.e., notifying all users who are "involved" in an issue, in case Flag integration is not available or not enabled.

That said, this turned the logic to figure out who to notify in mail.inc into a mind-boggling operation. I'm relatively sure that someone will ask "Can't we retrieve this with less database queries?", to whom I'm going to answer that the current code is at least reliable. ;) Performance optimization should only happen after adding unit tests, I think.

Sweetness, thanks! Have you looked into if/how this conflicts with #15380: Allow to configure content of issue notification e-mails?

I grabbed the patch, so I'll try to review on the plane.

Thanks again for running with all this, sun!!!

Cheers,
-Derek

1. Renamed {project_subscriptions} into {project_issue_subscriptions_project}
2. Renamed {project_user_subscriptions} into {project_issue_subscriptions_user}
3. Renamed form, theme, and function callbacks equally to be in the project_issue_subscriptions namespace.

Only as a preparation for moving issue subscriptions/notifications into a separate sub-module at some point in the future. That task won't be part of this patch. After discussion with @dww in IRC, renaming the tables always was on the todo list, and since we're touching the code anyway here, we can happily account for that.

Okay, haven't boarded yet, so here's a first pass review based solely on looking at the patch. ;) Thanks again for working on this!

A) I'd love to have constants for the subscription levels.

B) Might need to denormalize {users}.status into {project_issue_subscriptions_project} for this one:

+ WHERE u.status = 1 AND pisp.level = 2 AND pisp.nid = %d AND pisp.uid IN ($placeholders)", $args);

and into {project_issue_subscriptions_user} for this:

+ $result = db_query("SELECT pisu.uid, u.name, u.mail FROM {project_issue_subscriptions_user} pisu INNER JOIN {users} u ON pisu.uid = u.uid WHERE u.status = 1 AND pisu.level = 2");

But I agree, those kinds of optimizations can either wait for more tests, or at least for EXPLAIN testing/review.

C) Comment doesn't seem to match code:
+ // Retrieve all users who flagged the issue.
+ $flag_contents = flag_get_content_flags('node', $node->project_issue['pid'], variable_get('project_issue_subscription_flag', 0));

Isn't that all users that flagged the project node for the issue you're looking at?

D) I don't know that "flagged" issues is the best in the UI for lists like this:

+function _project_issue_subscription_levels() {
+ return array(
+ 0 => t('None'),
+ 1 => t('Flagged issues'),
+ 2 => t('All issues'),
+ );
+}

Isn't that going to be confusing for the not-using-flag case? ;) Maybe call it "subscribed issues" and then that can mean different things depending on what's installed and configured? I was going to say leave it "own issues" but that seems too confusing with issues you actually created vs. subscribed/assigned to? This could all use a more thorough UI/UX review.

E) I'm not sure what this means:
+ '#description' => t('Per-project subscriptions having the same or a lower level than the global level will be removed.'),
This part needs UX review for sure.

F) @see project_get_nid_from_uri():
+ if (!is_numeric($project_nid)) {
+ $project_nid = db_result(db_query(db_rewrite_sql("SELECT p.nid FROM {project_projects} p WHERE p.uri = '%s'", 'p'), $project_nid));
+ }

G) I'd much rather we had an API for manipulating subscription stuff and the form submit handlers just unloaded the form and called the right API function, instead of doing all the queries directly in the form submit handlers.

H) This is perhaps a misleading/unfortunate path for this page:
+ $items['user/%user/subscribe-mail'] = array(
Ultimately, I'd like to merge this with /user/%user/notifications (or wherever) as per #15380: Allow to configure content of issue notification e-mails.

I) I don't really buy this:
+ // We only allow to change (and remove) per-project subscriptions on this
+ // form. Users are able to subscribe to further projects by visiting the
+ // individual project pages. In terms of UX, that's preferred anyway, since
+ // a user normally wants to know and be sure what exactly she subscribes to.
I don't see anything wrong with a blank row at the bottom to add a new one (with an autocomplete title field and a level selector). We use that basic UI all over the place, and I think it'd be really nice to be able to add other projects you know you want to start following without having to navigate to those project pages, click around to find the hidden "Subscribe" link, etc. If it was really slick, there'd be an "Add another" AJAX button ala CCK with multi-valued fields. ;) I'd be happy to punt both of these to separate issues, not blockers for a commit.

That's all I saw on visual inspection. I haven't tried applying or using it yet, but this should keep you busy for a little while. ;) Thank you for bringing such beautifully-written code into Project*!

Cheers,
-Derek

1. Added constants.
2. @todo EXPLAIN
3. Fixed nid of flag lookup.
4. Renamed "Flagged issues" into "Subscribed issues" in the form options.
5. Re-phrased the description into "All project subscription levels with the same or lower level as your global default level will be removed."
6. Changed to use project_get_nid_from_uri()
7. @todo API functions
8. Changed into user/%user/subscriptions
9. @offtopic Autocomplete for adding new project subscriptions. Separate issue.

Added API functions for project issue subscriptions user settings.

I'm not sure whether it's really worth to also add API functions for project subscription settings.

Changed the user settings form to be compatible with #15380: Allow to configure content of issue notification e-mails and generally extensible.

In other words, this is the final patch.

Wow, awesome progress, thanks! I'm on my layover now, so I just downloaded all the latest patches. I'll try to give this another close look either while I wait for my flight, or while I'm on the plane to SFO.

Re: E/5: I still think that's confusing. I'll ponder better help text myself, but if any UX/UI folks have other ideas, lets hear them! ;) Also, I'm not sure this behavior is exactly what we want. Even though the who-gets-what email logic makes sense like this, I'm worried about someone who is manually subscribed to "own issues" in 30 projects, accidentally says they want all emails on all projects, quickly sets that one radio back to how they had it, and all their individual subscriptions are already gone. :( Is there huge harm in leaving the per-project data in the DB if the global setting changes? If we're going to do the destructive operation, I almost think we need a confirm form for setting the global setting or something. This could also go to a separate issue perhaps, but again, I want to raise it here for consideration.

Re: G/7: Even though you wouldn't know this from looking around at the rest of the Project* codebase, but I'm a firm believer that you *always* want API functions to do stuff, and form submit handlers to invoke those functions. ;)

Re: H/8: <bikeshed>user/%user/notifications? .../subscriptions? .../[other?]?</bikeshed> ;)

Anyway, not setting this to "needs work" over any of these issues...

p.s. hunmonk is obviously not working on this, unassigning. If anyone, sun should claim this.

Finally managed to install the drupalorg_testing profile after some hours of headdesking...

• Added API functions for project subscriptions.
• Removed the possibly confusing behavior that unsubscribed from projects when global default was a same or higher level than project level.
• Fixed form constructors and submission handlers.
• Fixed database table indexes.

Now some funky screenshots:

whoopsie, #25 contains some unintended upstream changes. ;)

And of course, that user account subscriptions screenshot in #25 is outdated, too :( Here's the current:

sub

Haven't read through the patch yet, but here's a couple of thoughts that stand out:

1) It'd be good to change the page titles for this stuff to say "Notifications" or "Email notifications" instead of subscriptions since we're now apparently applying the subscribed terminology to a user manipulated list of content. So in essence some of the screenshots are effectively reading as "I'm subscribed to my subscribed issues" or "For this project I'm not subscribed to my subscribed issues..." This double usage of the subscriptions term is quite confusing, and although I'm not sure which to change, I think I'd favor changing the page titles/tabs to "Notifications"

@sun: first off, nice patch :) most of this looks great. some potential cleanup stuff / questions:

1. i think the check for a level of PROJECT_ISSUE_SUBSCRIPTIONS_NONE should be done in project_issue_subscriptions_project_setting_save(), this way it's at the same level as the fix up in project_issue_subscriptions_project_setting_load(), ie, as close to the db call as possible. i would like a more general project_issue_subscriptions_project_setting_update() function that handles all of the checks and queries necessary for the 'save it/delete it' logic we're using here -- that way the approach self-documents in one function.
2. same for project_issue_subscriptions_user_settings_save() if necessary?
3. <bikeshed>i think user/%user/subscriptions as a menu path is begging for a conflict. i'd go with user/%user/issue-subscriptions or user/%user/project-issue-subscriptions</bikeshed>
4. Re: // @todo Isn't this globally enforced elsewhere...?, i think that might be a good idea to keep in the case of users registered via external user auth mechanisms?
5. '#options' => $form['project_issue_subscriptions']['level']['#options'], feels more confusing than just simply using '#options' => _project_issue_subscription_levels(), as you do earlier in the form.
6. is there some reason for '#parents' => array('project_issue_subscriptions', 'projects', $project->nid), instead of doing $form['options']['#tree'] = TRUE;?
7. Re: // We only allow to change (and remove) per-project subscriptions on this form., i agree w/ dww that we should allow users to add new projects here via an autocomplete form. however, i don't think it's enough to hold up the initial commit of the patch, so add it if you like, or we can save it for a followup issue.
8. as mentioned previously, we need to work over those queries in the mailer code before this can be RTBC.
9. also agree w/ mikey_p's comments in #29

Thanks for the reviews, @mikey_p + @hunmonk!

1. @hunmonk is right that both user/%/subscriptions and user/%/notifications are a namespace conflict with Subscriptions module and Notifications module, which may be installed on a site next to project_issue. Therefore, I've changed the router path to user/%/project-issue.
2. Changed the local task/tab title to "Notifications". Also changed the form labels and descriptions accordingly to say "notifications".
3. Merged project_issue_subscriptions_project_setting_delete() into project_issue_subscriptions_project_setting_save(), acting accordingly depending on PROJECT_ISSUE_SUBSCRIPTIONS_NONE. Did not rename _save() to _update(), since _load() and _save() is a generic and consistent function naming pattern.
4. Did not change project_issue_subscriptions_user_settings_save() accordingly, since #15380: Allow to configure content of issue notification e-mails will extend the per-user table with further properties, so we never going to delete a row in that table.
5. Replaced the @todo with a clarifying inline comment.
6. Added $options variable for subscription level options in the form.
7. Clarified why #parents is used.
8. I want to leave adding new project-subscriptions via autocomplete to a separate issue. Personally, I wouldn't use that, as already discussed in #20 and explained in code: "In terms of UX, that's preferred anyway, since a user normally wants to know and be sure what exactly she subscribes to." -- in other words: I haven't seen an issue tracker in the wild that allows this, the idea and feature proposal sounds completely hypothetical to me, and I'd seriously wait for an actual feature request from a user, before attempting to implement this.

9. we need to work over those queries in the mailer code before this can be RTBC

   Can we clarify what needs to be done?

New screenshot of user tab:

+++ b/project_issue.module
@@ -884,6 +907,137 @@ function _project_issue_followup_get_user() {
+/**
+ * Implements hook_user().
+ */
+function project_issue_user($op, $edit, $account) {
+  if ($op == 'delete') {

We should also act when an account is blocked; otherwise, PI may turn into an UBE bot. Fixed in attached patch.

ah, the joys of working with the drupal.org database...

i never made it past testing the upgrade path, as there are some problems when trying to apply it to the drupal.org data:

1. {project_subscriptions} has over 2000 duplicate key entries, so the attempt to add the primary key to {project_issue_subscriptions_projects} fails. this is mostly due to one user have a bunch of duplicate entries -- however, since we can't be sure why these duplicate entries happened, it seemed safer to just write an update function to handle removal of duplicates
2. even after we clear out the level = 0 rows, {project_issue_subscriptions_project} still has over 2 million rows. adding a new primary key on that table is looking like a ridiculously long operation -- it's really making my dev machines struggle. this is something we need to be concerned about, as we'll want to avoid long drupal.org downtimes if possible. i'll want to check w/ nnewton on the best approach, and we may need to adjust the update functions to accomodate.

at any rate, attached patch has my current fixes to the upgrade path, which at least works now. :)

re: #31-9, it seems to me that working these queries against the current drupal.org subscriptions data would be a reasonable way to performance test. it would also be a good idea to run EXPLAIN on the queries just to make sure they are as optimized as they can be. it occurred to me that all the queries in that bunch are pulling the same data, just with different WHERE clauses -- might it be more efficient overall to build up the WHERE clauses then issue one super ugly query?

hunmonk
nnewton: if adding a primary key doesn't block any other activity, we could just comment out that update query, run the full upgrade, fire the site back up then run it.  that seem reasonable?

nnewton
hunmonk, adding a PK will lock the table

nnewton
I meant, we could run the update/pk add on a dev site and see how long it takes
then schedule a downtime or not from there, since the dev sites are on similar hardware

hunmonk
nnewton: and there's no faster way to do it than with ALTER TABLE?

nnewton
hunmonk, nope,. thats the fastest way

so i guess we've got the best approach. attached patch adds a warning to the update function in question -- i guess that's the best we can do in this situation.

• removed the check for n.type = 'project_project' in the query that pulls per-project subscription settings on the user page. we don't need this, as {project_issue_subscriptions_project} only has projects in it.
• whitespace and comment cleanups in upgrade path functions.
• reworked project_issue_subscriptions_user_settings_save() to behave like the per-project save function. there's no reason to store PROJECT_ISSUE_SUBSCRIPTIONS_NONE levels in here, either.
• further clarified code comments for project_issue_subscriptions_user_settings_save() and project_issue_subscriptions_user_settings_load()
• rolled back the change from #32. i would need clarification what a UBE bot is (no love from google). furthermore, it makes sense to me that a blocked user's subscriptions should be preserved, they should just not receive notifications, and the queries in the mailer logic already attend to this. i've filed #1237056: project_issue_project_maintainer_project_load() should filter out blocked users to handle the inconsistency with loading issue maintainers.

i've tested all the form code pretty well, and i'm satisfied with it. still haven't tested the mailer logic or flag integration, but the below explains are a good first step to analyzing the related queries:

EXPLAIN SELECT pisu.uid, u.name, u.mail
        FROM project_issue_subscriptions_user pisu
        INNER JOIN users u ON pisu.uid = u.uid
        WHERE u.status = 1 AND pisu.uid IN (1, 2) AND pisu.level = 1;

+----+-------------+-------+--------+-------------------+-----------+---------+--------------------+------+--------------------------+
| id | select_type | table | type   | possible_keys     | key       | key_len | ref                | rows | Extra                    |
+----+-------------+-------+--------+-------------------+-----------+---------+--------------------+------+--------------------------+
|  1 | SIMPLE      | pisu  | index  | PRIMARY,uid_level | uid_level | 5       | NULL               |    1 | Using where; Using index |
|  1 | SIMPLE      | u     | eq_ref | PRIMARY           | PRIMARY   | 4       | drupalorg.pisu.uid |    1 | Using where              |
+----+-------------+-------+--------+-------------------+-----------+---------+--------------------+------+--------------------------+

EXPLAIN SELECT pisp.uid, u.name, u.mail
       FROM project_issue_subscriptions_project pisp
       INNER JOIN users u ON pisp.uid = u.uid
       WHERE u.status = 1 AND pisp.nid = 34309 AND pisp.uid IN (1, 2) AND pisp.level = 2;
+----+-------------+-------+--------+-------------------------------------------------+---------+---------+--------------------+------+-------------+
| id | select_type | table | type   | possible_keys                                   | key     | key_len | ref                | rows | Extra       |
+----+-------------+-------+--------+-------------------------------------------------+---------+---------+--------------------+------+-------------+
|  1 | SIMPLE      | pisp  | range  | PRIMARY,project_subscriptions_nid_uid_level,uid | PRIMARY | 8       | NULL               |    2 | Using where |
|  1 | SIMPLE      | u     | eq_ref | PRIMARY                                         | PRIMARY | 4       | drupalorg.pisp.uid |    1 | Using where |
+----+-------------+-------+--------+-------------------------------------------------+---------+---------+--------------------+------+-------------+

killes asked for an EXPLAIN on {project_issue_subscriptions_user} with the table populated with some data. i simply populated it with all users from {users}, all with level = 0:

mysql> EXPLAIN SELECT pisu.uid, u.name, u.mail
    ->         FROM project_issue_subscriptions_user pisu
    ->         INNER JOIN users u ON pisu.uid = u.uid
    ->         WHERE u.status = 1 AND pisu.uid IN (1, 2) AND pisu.level = 1;
+----+-------------+-------+--------+-------------------+---------+---------+--------------------+------+-------------+
| id | select_type | table | type   | possible_keys     | key     | key_len | ref                | rows | Extra       |
+----+-------------+-------+--------+-------------------+---------+---------+--------------------+------+-------------+
|  1 | SIMPLE      | pisu  | range  | PRIMARY,uid_level | PRIMARY | 4       | NULL               |    2 | Using where |
|  1 | SIMPLE      | u     | eq_ref | PRIMARY           | PRIMARY | 4       | drupalorg.pisu.uid |    1 | Using where |
+----+-------------+-------+--------+-------------------+---------+---------+--------------------+------+-------------+

#35 didn't apply to master (not sure why not). Here's a re-roll that fixes the rejected hunk in mail.inc and removes the other patch fuzz. I'll do more thorough review and testing in a bit, just wanted to post this here for now.

Argh, sorry, ignore #37. My local workspace was on another branch. ;)

ok, total rewrite of the queries to pull the recipient list. it's now only two queries no matter what, and i think the entire coding approach is a bit cleaner and less confusing. Here's some relevant EXPLAINS:

mysql> EXPLAIN SELECT pi.uid, u.name, u.mail
    ->     FROM project_issue_subscriptions_user pi
    ->     INNER JOIN users u ON pi.uid = u.uid
    ->     WHERE u.status = 1 AND ((pi.level = 2) OR (pi.uid IN (1, 2) AND pi.level = 1));
+----+-------------+-------+--------+-------------------+-----------+---------+------------------+--------+--------------------------+
| id | select_type | table | type   | possible_keys     | key       | key_len | ref              | rows   | Extra                    |
+----+-------------+-------+--------+-------------------+-----------+---------+------------------+--------+--------------------------+
|  1 | SIMPLE      | pi    | index  | PRIMARY,uid_level | uid_level | 5       | NULL             | 921520 | Using where; Using index |
|  1 | SIMPLE      | u     | eq_ref | PRIMARY           | PRIMARY   | 4       | drupalorg.pi.uid |      1 | Using where              |
+----+-------------+-------+--------+-------------------+-----------+---------+------------------+--------+--------------------------+

mysql> EXPLAIN SELECT pi.uid, u.name, u.mail
    ->     FROM project_issue_subscriptions_project pi
    ->     INNER JOIN users u ON pi.uid = u.uid
    ->     WHERE u.status = 1 AND pi.nid = 34309 AND ((pi.level = 2) OR (pi.uid IN (1, 2) AND pi.level = 1));
+----+-------------+-------+--------+-------------------------------------------------+-------------------------------------+---------+------------------+------+--------------------------+
| id | select_type | table | type   | possible_keys                                   | key                                 | key_len | ref              | rows | Extra                    |
+----+-------------+-------+--------+-------------------------------------------------+-------------------------------------+---------+------------------+------+--------------------------+
|  1 | SIMPLE      | pi    | ref    | PRIMARY,project_subscriptions_nid_uid_level,uid | project_subscriptions_nid_uid_level | 4       | const            |  983 | Using where; Using index |
|  1 | SIMPLE      | u     | eq_ref | PRIMARY                                         | PRIMARY                             | 4       | drupalorg.pi.uid |    1 | Using where              |
+----+-------------+-------+--------+-------------------------------------------------+-------------------------------------+---------+------------------+------+--------------------------+

+++ b/project_issue.module
@@ -939,25 +939,38 @@
+ *     When passing PROJECT_ISSUE_SUBSCRIPTIONS_NONE the user's per-user
+ *     subscription setting is deleted.
...
 function project_issue_subscriptions_user_settings_save($account) {
...
+  $level = $account->project_issue_subscriptions['level'];
+  if ($level > PROJECT_ISSUE_SUBSCRIPTIONS_NONE) {
...
+  else {
+    db_query("DELETE FROM {project_issue_subscriptions_user} WHERE uid = %d", array(

As mentioned earlier, this was purposively not done, because #15380: Allow to configure content of issue notification e-mails will add further per-user subscription setting columns to this table.

@sun: whoops, missed that comment. i'll fix my error on the next re-roll.

i tested the results of the new recipient list queries extensively, all levels, for both projects and users, with both the flag module approach and the default approach, and everything appears to be working perfectly.

in particular:

• users that are not subscribed at either the user or project level receive no emails, even if the issue is flagged.
• users do not get emails on non-flagged issues even if they are set to 'own'
• users always receive email regardless of flag state if they are set to 'all'

couple of things left to consider:

1. these new queries, while fewer, are a bit more complicated, and i'm guessing we'll need to play w/ the order or add some indexes to satisfy efficiency requirements
2. the new recipient list approach reduces the difference between the default approach and the flag approach to "what is the list of users for this issue that needs to be considered for the 'own' case". because of this, i think it would be quite easy to use a hook_project_issue_mail_subscribed() or some similar approach to make the method of determining that list of users pluggable, instead of hard coding a check for flag module. i'd prefer to do this actually. the hook could just return an internal name mapped to a callback function, then the project issue setting could be used to pick that. then we break out the current flag-specific functionality into a mini project_issue_subscribe_via_flag module and we have a much better abstraction than the current approach. thoughts?

@hunmonk: Excellent, glad this is working so nicely. Once you re-roll for the settings fix, I'm planning to deploy this on subscribe-drupal.redesign.devdrupal.org to get some wider testing (and pound on it myself).

That said, let's not overcomplicate this with the proposed new hook. We can always add that complexity later if we need it, but I'd much rather just get this in, even if it's got some if (module_exists('flag')) { // stuff } code in it. This is only the first of many patches to address this area of the code, and I neither want to make this larger and more complex than it needs to be, nor write code now that we think we might want later. Let's write that code when we actually need it (if that day ever comes), since we'll have a better idea exactly what it needs to be once we have a real use-case for it.

Thanks,
-Derek

Only yesterday, I realized that I'm going to lose all of my per-issue subscriptions on core issues due to this patch. So I wondered a) whether this is good or bad in the first place ;) and b) how complex it would be to find all uids that have pisp.level = own (pisu.level does not exist yet), and for those nids + uids, find comments, and inject flag records.

+++ b/project_issue.install
@@ -626,3 +652,105 @@ function project_issue_update_6006() {
+ * Clean up duplicates in {project_subscriptions}.
+ */
+function project_issue_update_6007(&$sandbox) {
...
+ * Ensure that existing project subscriptions are clean.
+ */
+function project_issue_update_6009() {
+  $ret = array();
+  $ret[] = update_sql("DELETE FROM {project_issue_subscriptions_project} WHERE level = 0");

Why don't we clean out zeros before checking for duplicates?

@sun re: #43:

Good point on the order of the DB updates. In fact, since they're all going in with the same patch, why not just put the query from 6009 directly inside the initialization block of 6007? I guess we can keep the other two separate, especially if what's now 6010 takes insanely long to run.

However, the plan all along at #34496: [meta] Add Flag module to allow users to subscribe/unsubscribe without posting a comment has been that once we're actually migrating to flag, we'll initially populate the flag table via the author and everyone who's commented on each node. We're going to need that for "Your Issues" to keep working on all projects, regardless of your email notification settings.

p.s. Should be obvious, but the exact query would be modified since the {project_subscriptions} table won't have been renamed yet...

• fixed the error sun mentioned in #40
• moved update 6009 to 6007 and renumbered updates accordingly. i don't see an issue with leaving this in it's own update, the query initializing 6008 is also a little expensive and i'd rather not pile more stuff in there.
• better doxygen for update 6009
• further tweaking of the indexes/queries on {project_issue_subscriptions_user} and {project_issue_subscriptions_project} -- they are definitely performing better, but i'm still waiting on my dev machine to finish putting in another index for the final tweak. will post EXPLAINs when i can.

@all: any thoughts as to where we would put the code that handles the mapping of old subscriptions to flagged issues? clearly it can't be a project_issue update, but it could be useful for other project* installs. mini-module?

ok, new EXPLAINs for the query tweaks in #46. i'm pretty happy with these now, and don't have any more optimizations i can think of, so offer them up if i missed something -- both queries are MUCH snappier:

mysql> EXPLAIN SELECT pi.uid, u.name, u.mail
    ->      FROM project_issue_subscriptions_user pi
    ->      INNER JOIN users u ON pi.uid = u.uid
    ->      WHERE u.status = 1 AND ((pi.level = 2) OR (pi.level = 1 AND u.uid IN (1, 2)));
+----+-------------+-------+--------+---------------+---------+---------+------------------+------+--------------------------+
| id | select_type | table | type   | possible_keys | key     | key_len | ref              | rows | Extra                    |
+----+-------------+-------+--------+---------------+---------+---------+------------------+------+--------------------------+
|  1 | SIMPLE      | pi    | range  | PRIMARY,level | level   | 1       | NULL             |    2 | Using where; Using index |
|  1 | SIMPLE      | u     | eq_ref | PRIMARY       | PRIMARY | 4       | drupalorg.pi.uid |    1 | Using where              |
+----+-------------+-------+--------+---------------+---------+---------+------------------+------+--------------------------+

mysql> EXPLAIN SELECT pi.uid, u.name, u.mail
    ->      FROM project_issue_subscriptions_project pi
    ->      INNER JOIN users u ON pi.uid = u.uid
    ->      WHERE u.status = 1 AND pi.nid = 34309 AND ((pi.level = 2) OR (pi.level = 1 AND u.uid IN (1, 2)));
+----+-------------+-------+--------+-------------------+-----------+---------+------------------+------+--------------------------+
| id | select_type | table | type   | possible_keys     | key       | key_len | ref              | rows | Extra                    |
+----+-------------+-------+--------+-------------------+-----------+---------+------------------+------+--------------------------+
|  1 | SIMPLE      | pi    | range  | PRIMARY,nid_level | nid_level | 5       | NULL             |  983 | Using where; Using index |
|  1 | SIMPLE      | u     | eq_ref | PRIMARY           | PRIMARY   | 4       | drupalorg.pi.uid |    1 | Using where              |
+----+-------------+-------+--------+-------------------+-----------+---------+------------------+------+--------------------------+

couple of additional thoughts on implementation of this patch, and deployment on drupal.org:

• i'd still like to advocate for a Project Issue Flag Integration module. we can skip the hook, and just use if (module_exists('project_issue_flag')) {} for now. it shouldn't be that much extra work to abstract it out, it's forward-looking, and it gives us a sensible place to put the batch function to import existing subscriptions into flag.
• since #15380: Allow to configure content of issue notification e-mails is inevitable, and we're always keeping rows in {project_issue_subscriptions_user}, why don't we just pre-populate that table now with all users at subscription level 'None', change the update/insert logic on save to just update, and use hook user's insert op to add new rows to the table? this feels cleaner to me.
• the indexing horrors on {project_issue_subscriptions_project} could result in significant downtime during the upgrade. i was thinking alternatively that we could:
  
  • disable issue subscriptions on d.o and put up a maintenance message just for that.
  • run the updates on a dev site, then dump in tables manually to d.o
  • take down d.o just long enough to load in the new code and manually push the schema_version

  This also offers us the opportunity for a quick rollback if the d.o upgrade demons throw us a monkey wrench.

First of all, huge thanks to sun and hunmonk for getting this so far along!

Some updates:

- I had hunmonk push a feature branch for this upstream, which now lives at 397458-flag-notification-email -- that includes everything from sun and all of hunkmonk's edits up to comment #46.

- I've pushed two commits since then:
http://drupal.org/commitlog/commit/1894/7cd94c9745a1f9838f686c82173d7bf8...
http://drupal.org/commitlog/commit/1894/4cb08dbd63ce0329ce98ccebbb4027e2...
The commit messages explain what happened. The most critical bug I fixed was menu access for user/N/project-issue. ;)

- Limited local testing seems to work well. However, this still needs more thorough testing before I can deploy it.

- I think the UI at user/N/project-issue is terrible. ;) I'm working with yoroy in IRC right now to try to come up with something better. I can't roll this out like this. ;) But, we're getting very close here.

I'm actively working on designing and fixing the UI, so I'm assigning to myself. I'll update here when there's more news.

Okay, this almost works and seems sane to me. It's basically what I proposed to yoroy which he seemed more or less happy with. Currently in the branch, the UI for this looks like:

My new UI looks like this:

The attached patch applies cleanly to the end of the feature branch. I didn't just commit and push since I wanted a bit more feedback on this basic approach first. To make the UI more self-evident, I've changed the underlying behavior of the code. Instead of the global per-user setting being a master override, it now acts as a default value for any projects that don't have a per-project setting defined. That required changing the query logic for who gets a given email. The new query now involves a LEFT JOIN, since I couldn't come up with another way to get the behavior we want. However, the explain for that doesn't look too bad:

p.s. Now that the global setting is a default for all other projects, we should let you set the notification level to "None" if you actually wanted to over-ride the default (e.g. "own issues") to shut up emails from a specific project. That all works (at least in the UI, I haven't 100% tested all the mailing logic yet).

However, that means there's no longer any way to remove rows from this table. :( I'm leaning towards adding an "Operations" column with "delete" links. It seems a bit ugly, but at least it's consistent with other Drupal UIs.

Thoughts?

Thanks,
-Derek

I spoke with dww in IRC about this a bit and I think the UI (including the notion of an "operations" col) is quite straightforward and a HUGE improvment. I had my non-technical wife explain to me what all the options meant in the screenshot and she scored 100%. Thats an excellent sign.

+1 from me on #51 & #52

In IRC bleen asked what happens if you type in "Views" and you already have a row for views. My local version already has form validation for this:

I've also added code so you can pre-fill this row via a GET parameter, so that for example, on the per-project pages when you click on "Subscribe" (or equivalent) you can land on this page with the project pre-filled. I'm trying to figure out what the fate of pages like http://drupal.org/project/issues/subscribe-mail/views should be. I'm thinking to rip out that whole form and just have a redirect that sends you to your profile page with the project in question pre-filled and a destination set to send you back to the project page or issue queue you were looking at.

http://drupal.org/commitlog/commit/1894/1edeeae2eef1188e616efd00772a1349... -- adds delete links to an "Operations" column.

Here's what you see when you land on the page:

Here's what happens if you click a link:

I'm just using drupal_valid_token() to prevent CSRF -- this is a trivial thing to recover from, so a confirm form seemed pointless and annoying.

Still working on the replacement for the per-project subscription management UI...

Quick initial thoughts:

- Maybe show the default first so that you know what you'll be deviating from when overriding per project
- I think the autocomplete should be the very last row in the table.
- It is an autocomplete because these can be any project or is this already limited to your own projects? Otherwise a select list might be a bit more concrete and simpler to use here. If it is indeed any project (looooong list) than autocomplete is fine.
- Might want to add a placeholder text in the autocomplete like "Enter a project name…"

- My two concerns about having the default first are:
1) Having it at the end seems to reinforce the notion that it's the "catch all" default for anything not already in the table.
2) Not sure what the text would say, since "Default for all other projects" is weird if it's the first thing in the table. ;)
If we can address those somehow, I'm happy to move it.

- Sure, we can move the row to add a new project to the end. I kind of like it like it is here, but I agree an "add another" row at the very bottom is more consistent.

- Autocomplete since you can add notification settings for any project, not just the ones you own and/or maintain.

- Sure, the placeholder text could be nice. Not sure how to do that via FAPI. I think that has to be some magic in the theme.

Thanks for the review!

Dww has a demo site where you can test drive this functionality: http://subscribe-drupal.redesign.devdrupal.org/

Screenshot description (applies to previous 4 images in here as well):
Shows the 'Notifications' screen under 'Profile' on your account page on d.o.
There's a fieldset labeled 'Issue notifications' containing a table with 3 columns 'Project', 'Notification level' and 'Operations'. Initial state shows an autocomplete text field in the project cell of the first row. Type a project name to add it as a new row. Notification levels are offered in a select list with the options 'None', 'Subscribed issues' and 'All issues'. The only operation is 'delete'
The last row has 'Default for all other projects' in the 'Project' cell, the same options for notification level and no 'delete' operation link (obviously).

Some more comments on this interface:

- Nowhere is the word 'email' used. We might want to do that :)
- No need for a fieldset around the form if this is the only thing living on this page.

re: 'e-mail': yeah, I was thinking of that, too. ;) Indeed, we should probably mention that word.

re: single fieldset: yeah, we know. ;) this whole patch is being written assuming that #15380: Allow to configure content of issue notification e-mails is going to be finished and deployed at the same time, which will add another fieldset to the page. If disaster strikes and that's not ready for some reason, it's a few lines to just take the fieldset out of this one, but I'd rather just leave it in for now.

Note: the middle choice in the dropdowns is actually "Issues you follow" since this site is configured with the flag.module integration stuff. The other screenshots I posted are from a local test site to test the non-flag case, so those show "Own issues" instead...

placeholder text can be added with the "elements" module (or the "placeholder" module, but Im not really supporting it anymore) ... I dont know how well that will work with an autocomplete filed though.

As for calling the tab "email notifications", isn't this tab also going to be used to manage other settings that are not necessarily related to email? Unfortuneltey I dont have a great alternate suggestion for getting the work "email" in there. In fact the only thing i can think of is "Issue notifications (email)" as the fieldset title. It works but its not great...

1) For the placeholder, I suggest we just simply and blatantly ignore older browsers as well as XHTML validation errors and use this one-liner:

  '#attributes' => array('placeholder' => 'Enter a project name...'),

2) "e-mail" wasn't precisely stated here, since at some point, I thought there was a desire to re-use these project notification settings for something else... (Tracker2, Flag-based "my issues" or so?) Perhaps I'm mistaken.

3) The tab label should be kept short; even "Notifications" on its own is an awful long word already.

Re: 'placeholder' -- slick, I didn't even realize that was part of HTML5. I'm so behind the times. ;) So, on HTML5-compliant browsers, we don't need the whole elements module, we just need this:

Looks like this in chrome:

The attribute degrades fine and just looks like this in Firefox 3:

However, if we want it to work on older browsers, we need placeholder module (which is incredibly tiny). Not sure how much we care.

Either way, what do y'all think about "Enter a project title" for the actual placeholder text itself?

---

This fieldset is entirely about e-mail, so just "Issue e-mail notifications" would work for the title.

The one from #15380: Allow to configure content of issue notification e-mails is about preferences for those emails, so its fieldset could be called "Issue e-mail preferences" (or perhaps "Notification e-mail preferences").

Re: non-e-mail notifications: we're not sure yet. Currently that's all it's for, and I don't even know of any medium-term plans for other kinds of notifications on d.o that need settings like this. There are some long-term "plans" (more like dreams) involving d.o notifications that might need per-user settings, but those are definitely far from happening.

p.s. According to http://www.findmebyip.com/litmus/#mdz_input_placeholder the 'placeholder' attribute is already supported on all browsers that matter except IE. Typical. I think IE users can (continue to) suffer on this one. I'm leaning towards not trying to add IE-specific (and JS-only) hacks for this. Let's just put it in the markup and be done with it.

Oh, and I just saw sun and I cross-posted, fun. Hi sun! ;) Agreed on keeping the tab label itself short.

Re: other notifications: flagging issues to follow them *is* being used for other stuff besides e-mail notifications. See the updated summary at #34496: [meta] Add Flag module to allow users to subscribe/unsubscribe without posting a comment for more. However, this issue is just about the e-mail notification aspect of the whole effort. Furthermore, none of the other uses for the "follow" flag need any per-user settings or preferences.

All that said, while I'm leaving this "needs review", there's one big piece still missing on this branch -- what to do with the per-project "subscribe" pages. See the bottom of my comment #54 for my proposed plan. I'd love feedback on that before I spend more time implementing it.

Thanks everyone!
-Derek

I already discussed dww's proposal in #54 with him on IRC last night ... so for the record here, I dont think there is any reason to keep a separate page for the per-project subscribe pages if we can so easily direct users to the "notifications" tab on his/her user profile.

+1 for dww's proposal

it occurred to me that an admin could stupidly delete the notification flag and screw up the link that's made in this patch between flag and project_issue. we could add validation logic to flag's admin delete confirm form to prevent this, but i think a better overall approach would be to create our flag programmatically, which flag allows us to do.

this would provide a couple of nice benefits:

1. automagically set up the flag for correct usage
2. prevent the deletion problem mentioned above
3. smooth out the data migration process at mentioned over at #1284704-2: Provide a data migration path for following issues via flag.module.

Love it! I already had the flag exported to code, so that's the perfect place to put it. I'll do that when I'm back on my laptop.

Cheers,
-Derek

Bojhan is in favor of keeping the separate page for the per-project notification stuff, since we have no good way to highlight a given table row if we redirect users to the "Notifications" tab on their profile. That's actually nice since when I was looking at this, the redirect thing is a can of worms, especially since the link is cached in the "Issues for [X]" block.

Given the existing code in the branch, here's what the per-project "Subscribe" pages currently look like:

This could use a little help:

A) The description text doesn't need to replicate the page title. ;) And you don't necessarily have to "receive" notifications at this page. How about "Manage e-mail notifications for [project-title] issues" for the page title? Or maybe just "Manage e-mail notifications"?

B) Let's rip out this prefix help text:

"These settings apply to the Views project only. You can see and change settings for all projects in your user account."

And then just use this for the description text between the radios and the save button:

"These settings only apply to the Views project. You can manage notifications for all projects in your user account."

C) "Save preferences" instead of "Save" on the button text? Actually, since all you can do here is tweak this one value, perhaps "Save notification preferences"?

D) What should the link text that brings you to this page be? Currently, on the issue queue views it's just "Subscribe". When viewing a project page, the link in the "Issues for [project-title]" block is called "Subscribe via e-mail". Do we want "Manage notifications"? Something else?

---

Meanwhile, still no resolution on a few other questions on the user's "Notifications" preference tab:

E) How's "Enter a project title" for the placeholder on the text field for new projects?

F) How about calling the fieldset title "Issue e-mail notifications"?

G) We need to resolve what we're doing about the order of this table. I personally like it as-is, although we can try it with the "add another" row at the bottom, not the default. But if we put the catch-all default forst, we need to address my concerns in #57 above. We could also move the "Default for all other projects" row out of the table entirely into a separate setting again, but that seems more complicated for the UI over-all.

Input welcome, although I don't want this to drag on too much...

Thanks,
-Derek

A) "Manage email notifications for [project-title] issues" sounds good, I like the added specificity.
B) Good idea. Drop the "You can" at the start of the second sentence: "These settings only apply to the Views project. Manage notifications for all projects in your user account.
C) I think a plain "Save" is fine here
D) "Email notifications" ?
E) Good!
F) Nothing more concrete comes to mind. "Send email notifications for issues" maybe but yours is fine too I think
G)

- My two concerns about having the default first are:
1) Having it at the end seems to reinforce the notion that it's the "catch all" default for anything not already in the table.
2) Not sure what the text would say, since "Default for all other projects" is weird if it's the first thing in the table. ;)
If we can address those somehow, I'm happy to move it.

1) I think this comes down to simply choosing between two equally valid perceptions. I lean towards 'show defaults first (80%)', then get into the specifics for the rest (20%).
2) "Default" ? :)

Additional niggle I have is with the 'Notification level' label, it's a bit abstract. "Send email for" or "Send email" maybe?

re #68 g): the problem with having it at the bottom is that as the list of overridden projects gets longer, the default setting gets more de-emphasized, which is not what i think we want. if it's at the top, what about something like "Default for all projects (override individual projects below)", the part in parenthesis could be smaller.

Okay, I've fixed everything from 67/68. One remaining decision:

Re: G) I'm sold on putting the default first. However, I'm not sold on hunmonk's verbose suggestion from #69. Here are patches and screenshots of both choices for comparison:

verbose / hunmonk

terse / yoroy

To me, the first is a bit too verbose, but I fear the second isn't verbose enough. ;) Probably doesn't really matter and isn't worth holding this up over.

Meanwhile, here's what the per-project UI now looks like:

Much cleaner.

Verbose. The terse version, i have no idea what's happenin'. And it's not like it's horribly terse with 1 extra line.

Compromise approach:

?

As long as the "override invidual projects below" is there, I am good.

I think #72 works. The extra line sets this option apart visually as well.

Committed and pushed to the 34496-flag-integration feature branch. Yay! ;)

yay++

just a note: since we've added a default flag in the module code, i've pushed a commit to the 34496-flag-integration branch that makes this flag the default for the 'project_issue_follow_flag' variable:
http://drupalcode.org/project/project_issue.git/commit/9dd78ca

While working on #1284704: Provide a data migration path for following issues via flag.module I just had a slightly horrifying realization. On d.o, there are 2314905 rows in {project_subscriptions} that use level 1 ("own issues"). There are only about 8000 distinct users in this table at all. So, a lot of these users have hundreds (or for over 800 of them, thousands) of entries in this table saying "follow own issues" for separate projects (given the way this data used to be stored). These folks are going to end up with a very large table at user/N/project-issue, with no good way to bring sanity unless they click on thousands of "delete" links (at least there's no confirm form!).

Seems like we have a few options here (not mutually-exclusive):

A) Provide a "Reset notifications" button of some sort at the bottom of this page (with a confirm form) to remove *all* per-project notification settings.

B) Use a heuristic during the migration of this data that if a user has more than N rows in this table at level 1, set that as their global default and remove all the per-project settings at the same level. Not sure what N should be. Maybe 50? Some spot queries on the d.o database shows this seems like a pretty sane cut off. Certainly it'll ensure the UI isn't completely nuts. ;) If we use 50 as the cut-off, there are 1028 users with >= 50 rows at level 1. Those 1028 users eat up 2305786 of the 2314905 total rows in {project_subscriptions} at level 1. If we just convert those 1028 users into a per-user default for "own issues", we'll only end up with ~42000 total rows in {project_issue_notification_project} (~9000 at level 1, and ~33000 at level 2).

Furthermore, if we do this as a new update between what's now 6009 and 6010, we'll have a *much* smaller table for 6010 through 6012 (adding primary keys and indexes, and dropping an unused index). Those had been split out since they could take hours on a table with millions of rows. But, if we do the hard work to consolidate the data first, those should all run very quickly.

Anyone see any problem at least doing B? I'm less concerned about A if we do B, although A might also be useful. Just don't want to confuse the UI we worked so hard on. ;)

Thoughts?

Thanks,
-Derek

50 sounds sensible to me.

However, additionally, I'd suggest to also cut-off subscribers to "all issues".

Based on some internal data that @dww provided, there are 6 users who are subscribed to all issues of ~17,000 projects.

Only 1 of them looks ultimately "valid" to me after looking into the user accounts. That user is subscribed to all issues of 161 projects (CRAZY), and he's an active drupal.org user, contributing to core and contrib.

The other 5 look highly suspicious to me.

And now, we also need to take into account the patch in #1284716: drupal.org-specific tweaks for the per-user issue notification UI, which effectively disallows to subscribe to all issues in all projects on drupal.org.

Due to this analysis, my actual proposal would be to have a hard upper limit of 200 projects for which users can be subscribed to "all issues" during this migration. Everything above is just simply nuked and ignored.

Thanks for the analysis, sun! However, I think it's better to just handle those 6 users manually after this goes live instead of trying to complicate the upgrade path in the code any further. The heuristic for "own issues" makes sense, since that's a pretty sane global default. But I don't want to mess with special-casing the "all issues" users, especially since it's such a minority of the total rows in this table to worry about.

Also, forgot to tag this with "drupal.org notifications"...

#78.A now lives at #1295408: Provide a way to reset all per-user issue notification settings

Here's an untested patch for #78.B.

While waiting for #1292746: Refresh staging.d.o with live d.o data for testing flag integration and issue following I might try to reset http://subscribe-drupal.redesign.devdrupal.org to drop the new tables, insert a copy of the live {project_subscriptions} table from d.o and try these updates there...

+++ b/project_issue.install
@@ -747,13 +747,52 @@ function project_issue_update_6009() {
+  $query = db_query("SELECT uid, COUNT(nid) project_count FROM {project_issue_notification_project} WHERE level = %d GROUP BY uid HAVING project_count >= 50", PROJECT_ISSUE_NOTIFICATION_OWN);

1) s/\$query/$result/

2) For ANSI SQL compatibility, there should be an AS between COUNT(nid) and project_count.

+++ b/project_issue.install
@@ -747,13 +747,52 @@ function project_issue_update_6009() {
+  while ($account = db_fetch_object($query)) {
+    $uids[] = $account->uid;

$uid = db_result($query)

(No need to create objects of type stdClass here)

+++ b/project_issue.install
@@ -747,13 +747,52 @@ function project_issue_update_6009() {
+      $query = db_query("INSERT INTO {project_issue_notification_global} (uid, level) VALUES (%d, %d)", $uid, PROJECT_ISSUE_NOTIFICATION_OWN);

Did we rename this table from _user to _global?

Makes less sense to me. :P

+++ b/project_issue.install
@@ -747,13 +747,52 @@ function project_issue_update_6009() {
+      'query' => format_plural(count($uids), 'Converted per-project notification settings for @count users to per-user global defaults.', 'Converted per-project notification settings for 1 user a per-user global default.'),

I wonder whether this kind of update string 1) really needs to account for singular/plural forms, and 2) be translatable and translated...?

Thanks for the review, sun! Up late, I see. ;)

Re: _user vs. _global:

http://drupal.org/commitlog/commit/1894/7cd94c9745a1f9838f686c82173d7bf8...

- The two new tables are now {project_issue_notification_global}
and {project_issue_notification_project}. Both are per-user, the key
difference is global settings vs. per-project settings.

Re: format_plural() -- I always try to be conscious of making any user-facing text translatable. The work is already done, so I don't want to rip it out now. ;)

Otherwise, this patch fixes your other tweaks/concerns. It also fixes up a few of the code comments.

Sweet! Just tested this on http://subscribe-drupal.redesign.devdrupal.org:

1. Dropped the {project_issue_notification_project} and {project_issue_notification_global} tables.
2. Reset project_issue's schema version in {system} to 6006
3. mysqldump'ed live d.o's {project_subscriptions} table and imported that into this dev site's DB
4. ran drush updb

% time drush updb -y
...
real    1m40.072s
user    0m12.281s
sys     0m10.473s

Now that {project_issue_notification_project} ends up with just under 43K rows, updates 6011, 6012 and 6013 are blazingly fast. The entire migration for this stuff took 1 minute 40 seconds, and that's on the crappy dev site VM hardware. That's all extremely encouraging. ;)

I spot checked a number of users with different existing subscription settings and everything looks exactly as expected.

Given that sun reviewed and I tested (on the live data no less), I just committed/pushed:

http://drupal.org/commitlog/commit/1894/c67f609893b9be7f75c3e2fb0e802f30...

Back to fixed. ;)

Yay!

Awesome!!! This is amazing - thanks for all your work on this @hunmonk + @dww!

Now that I rolled the patch for #15380-41: Allow to configure content of issue notification e-mails and see this UI with an "Advanced settings" fieldset (http://drupal.org/files/15380-41.issue-notification-preferences.png) I'm no longer convinced that this "Issue e-amil notification" fieldset is a win. Our UI guidelines say you shouldn't have the primary interaction on a form live in a fieldset. However, since that fieldset title was the only place on the whole page that said the magic word "e-mail" I wanted to retain some kind of label/header. How's this look to folks? From the code side, it certainly simplifies the code and form structure to remove the additional nesting.

Thoughts?

Thanks!
-Derek

+++ b/includes/notification.inc
@@ -37,21 +37,19 @@ function project_issue_notification_user_form(&$form_state, $account) {
+  $form['issue_notification_header'] = array(
+    '#value' => '<h3>' . t('Issue e-mail notifications') . '</h3>',

Let's use a simple #type 'item' here; i.e., just change the #type and remove #collapsible.

+++ b/includes/notification.inc
@@ -125,14 +123,14 @@ function project_issue_notification_user_form(&$form_state, $account) {
   if (!empty($new_project['title'])) {
     $nid = db_result(db_query(db_rewrite_sql("SELECT n.nid FROM {node} n WHERE n.title = '%s' AND n.type = '%s'", 'n'), $new_project['title'], 'project_project'));
     if (empty($nid)) {

It looks like we're not validating yet whether the new project is in the table already?

+++ b/includes/notification.inc
@@ -125,14 +123,14 @@ function project_issue_notification_user_form(&$form_state, $account) {
-      $form_state['values']['project_issue_notification']['projects']['new']['nid'] = $nid;
+      $form_state['values']['projects']['new']['nid'] = $nid;

This should use form_set_value() instead.

Thanks for the review, sun!

Re: item vs. markup -- I think a <p> pretending to be a header is worse than the <h3> so I left that as-is.

Re: form_set_value() -- look at the comments on http://api.drupal.org/api/drupal/includes--form.inc/function/form_set_va... -- that API sucks. ;) I don't want to overly complicate this code to call a function that is just going to turn around and set the exact same thing into $form_state. Left that as-is, too.

So, I rerolled now that #15380-49: Allow to configure content of issue notification e-mails is committed and pushed, then committed this and pushed it:

http://drupal.org/commitlog/commit/1894/67a8357b5b49e1f701a3db3245d7b275...

Re: validation -- weird! ;) That code used to exist. See comment #54 above. I guess that got lost in some git shuffling. Restored that and also committed/pushed:

http://drupal.org/commitlog/commit/1894/51b50454e6c49042e720fd7bc9b541ce...

Calling this one fixed again. Hurray.

Thanks!
-Derek

p.s. I updated the code on http://subscribe-drupal.redesign.devdrupal.org to the latest from the '34496-flag-integration' branch again...

"(override individual projects below)"

At very first sight, this was very confusing to me, as I mistakenly read:

"(overrides individual projects below)"

Hence, I'd like to revise into:

"(used unless overridden for individual projects below)"

I am not sure, why this would need a description at all. Default, is something applied to everything, that isn't customized. I feel users generally understand it that way (although over riding is quite alienate concept). It seems like the description is adding more confusing than clarity, it seems to work counter effective - saying it works like you would expect (which then makes you question, whether that is what it does). Some tweaking to the text might solve this, but not having the text might even be better.

@#89 by sun on October 8, 2011 at 7:37pm:

"(override individual projects below)"

At very first sight, this was very confusing to me, as I mistakenly read:

"(overrides individual projects below)"

I had exactly the same confusion.

used unless customized?

I would suggest '(applies unless customized below)'. I agree with comments in #91 and #89 about confusion.

I would be okay with (applies unless customized below). It still sounds somewhat awkward to my mind, but I can't think of anything better.

'applies unless customized below' is basically explaining what 'default' means, which shouldn't be necessary. Best suggestion imo is then to remove it all together.

I'm happy to just remove it entirely and rely on people to understand that a default is a default unless you override it. However, see comments #69-#74. We already had this discussion once and at least chx complained that the terse version (see screenshots in #70) wasn't clear enough...

I'd like to not go around in circles on this.

How about 'global default' or 'side-wide default' as distinguished from the per-project settings which follow?

Either "Global default" or "Site-wide default" would be fine with me. I think it was the "Default for all projects" proposal in #69 that caused trouble (since "all projects" gets confusing). But "Global" or "Site-wide" should hopefully be clear. "Global" is maybe a little pretentious, as if Drupal.org was the whole globe. ;) So I have a slight preference for "Site-wide" -- but I'm okay with whatever yoroy and Bojhan think is best. ;)

After another IRC chat with yoroy, we decided to go with this:

Committed and pushed:

http://drupal.org/commitlog/commit/1894/3ced025183a77ee32d290ca1fb8b9824...

Also deployed on staging.d.o if anyone wants to play with it there...