Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
This comes from http://drupal.org/node/34496#comment-1329522
We need to write an action for sending email to everyone who flagged an issue node, the entire issue queue, or site-wide.
Comment | File | Size | Author |
---|---|---|---|
#99 | 397458-99.notifications-ui-tweaks.png | 39.13 KB | dww |
#89 | project_issue.notification-default-description.89.patch | 546 bytes | sun |
#86 | 397458-86.notification-ui-no-main-fieldset.patch | 5.5 KB | dww |
#86 | 397458-86.notification-ui-no-main-fieldset.png | 46.55 KB | dww |
#83 | 397458-83.migrate-per-project-to-per-user-settings.patch | 3.15 KB | dww |
Comments
Comment #1
quicksketchAs 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.
Comment #2
sun@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.
Comment #3
mitchell CreditAttribution: mitchell commentedI 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)
Comment #4
dwwI'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...
Comment #5
hunmonk CreditAttribution: hunmonk commentedthis is C from #34496: [meta] Add Flag module to allow users to subscribe/unsubscribe without posting a comment which i'm handling.
Comment #6
hunmonk CreditAttribution: hunmonk commentednow 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:
Comment #7
dwwFYI: 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
Comment #8
philbar CreditAttribution: philbar commentedtagging
Comment #9
anarcat CreditAttribution: anarcat commentedHi 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!
Comment #10
Fabianx CreditAttribution: Fabianx commented#397464: Action for executing a flag has been commited already a while ago.
Can't we use that one?
Best Wishes,
Fabian
Comment #11
pillarsdotnet CreditAttribution: pillarsdotnet commentedComment #12
xjmCan someone clarify whether #397464: Action for executing a flag resolves this, or (if not) the specific cases that are not covered?
Comment #13
dwwNo, #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.
Comment #14
sunFirst 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.
Comment #15
sunMissed a second instance of the obsolete/removed 'project_issue_global_subscribe_page' variable.
Comment #16
dwwWow, 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).
Comment #17
sunAttached 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.
Comment #18
dwwSweetness, 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
Comment #19
sunOnly 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.
Comment #20
dwwOkay, 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
Comment #21
sunproject_get_nid_from_uri()
user/%user/subscriptions
Comment #22
sunAdded 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.
Comment #23
sunChanged 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.
Comment #24
dwwWow, 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.
Comment #25
sunFinally managed to install the drupalorg_testing profile after some hours of headdesking...
Now some funky screenshots:
Comment #26
sunwhoopsie, #25 contains some unintended upstream changes. ;)
Comment #27
sunAnd of course, that user account subscriptions screenshot in #25 is outdated, too :( Here's the current:
Comment #28
Dane Powell CreditAttribution: Dane Powell commentedsub
Comment #29
mikey_p CreditAttribution: mikey_p commentedHaven'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"
Comment #30
hunmonk CreditAttribution: hunmonk commented@sun: first off, nice patch :) most of this looks great. some potential cleanup stuff / questions:
user/%user/subscriptions
as a menu path is begging for a conflict. i'd go withuser/%user/issue-subscriptions
oruser/%user/project-issue-subscriptions
</bikeshed>// @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?'#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.'#parents' => array('project_issue_subscriptions', 'projects', $project->nid),
instead of doing$form['options']['#tree'] = TRUE;
?// 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.Comment #31
sunThanks for the reviews, @mikey_p + @hunmonk!
user/%/subscriptions
anduser/%/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 touser/%/project-issue
.Can we clarify what needs to be done?
New screenshot of user tab:
Comment #32
sunWe should also act when an account is blocked; otherwise, PI may turn into an UBE bot. Fixed in attached patch.
Comment #33
hunmonk CreditAttribution: hunmonk commentedah, 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:
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?
Comment #34
hunmonk CreditAttribution: hunmonk commentedso 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.
Comment #35
hunmonk CreditAttribution: hunmonk commentedn.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.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:
Comment #36
hunmonk CreditAttribution: hunmonk commentedkilles 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:Comment #37
dww#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.
Comment #38
dwwArgh, sorry, ignore #37. My local workspace was on another branch. ;)
Comment #39
hunmonk CreditAttribution: hunmonk commentedok, 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:
Comment #40
sunAs 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.
Comment #41
hunmonk CreditAttribution: hunmonk commented@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:
couple of things left to consider:
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?Comment #42
dww@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
Comment #43
sunOnly 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.
Why don't we clean out zeros before checking for duplicates?
Comment #44
dww@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.
Comment #45
dwwp.s. Should be obvious, but the exact query would be modified since the {project_subscriptions} table won't have been renamed yet...
Comment #46
hunmonk CreditAttribution: hunmonk commentedComment #47
hunmonk CreditAttribution: hunmonk commented@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?
Comment #48
hunmonk CreditAttribution: hunmonk commentedok, 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:
Comment #49
hunmonk CreditAttribution: hunmonk commentedcouple of additional thoughts on implementation of this patch, and deployment on drupal.org:
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.This also offers us the opportunity for a quick rollback if the d.o upgrade demons throw us a monkey wrench.
Comment #50
dwwFirst 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.
Comment #51
dwwOkay, 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:
mysql> EXPLAIN SELECT pi.uid, u.name, u.mail
FROM project_issue_notification_global pi
LEFT JOIN project_issue_notification_project pinp ON pi.uid = pinp.uid AND pinp.nid = 103
INNER JOIN users u ON pi.uid = u.uid
WHERE u.status = 1 AND pinp.level IS NULL 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 |
| 1 | SIMPLE | pinp | eq_ref | PRIMARY,nid_level | PRIMARY | 8 | drupal-6.pi.uid,const | 1 | Using where; Not exists |
| 1 | SIMPLE | u | eq_ref | PRIMARY | PRIMARY | 4 | drupal-6.pi.uid | 1 | Using where |
+----+-------------+-------+--------+-------------------+---------+---------+-------------------------------+------+-------------------------+
3 rows in set (0.00 sec)
Comment #52
dwwp.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
Comment #53
bleen CreditAttribution: bleen commentedI 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
Comment #54
dwwIn 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.
Comment #55
dwwhttp://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...
Comment #56
yoroy CreditAttribution: yoroy commentedQuick 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…"
Comment #57
dww- 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!
Comment #58
yoroy CreditAttribution: yoroy commentedDww 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.
Comment #59
dwwre: '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...
Comment #60
bleen CreditAttribution: bleen commentedplaceholder 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...
Comment #61
sun1) For the placeholder, I suggest we just simply and blatantly ignore older browsers as well as XHTML validation errors and use this one-liner:
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.
Comment #62
dwwRe: '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:
'#attributes' => array(
'placeholder' => t('Enter a project title'),
),
(That's basically all elements.module does in this case -- just moves stuff from '#placeholder' to '#attributes').
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.
Comment #63
dwwp.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
Comment #64
bleen CreditAttribution: bleen commentedI 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
Comment #65
hunmonk CreditAttribution: hunmonk commentedit 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:
Comment #66
dwwLove 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
Comment #67
dwwBojhan 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
Comment #68
yoroy CreditAttribution: yoroy commentedA) "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)
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?
Comment #69
hunmonk CreditAttribution: hunmonk commentedre #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.
Comment #70
dwwOkay, 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.
Comment #71
chx CreditAttribution: chx commentedVerbose. The terse version, i have no idea what's happenin'. And it's not like it's horribly terse with 1 extra line.
Comment #72
dwwCompromise approach:
?
Comment #73
chx CreditAttribution: chx commentedAs long as the "override invidual projects below" is there, I am good.
Comment #74
yoroy CreditAttribution: yoroy commentedI think #72 works. The extra line sets this option apart visually as well.
Comment #75
dwwCommitted and pushed to the 34496-flag-integration feature branch. Yay! ;)
Comment #76
bleen CreditAttribution: bleen commentedyay++
Comment #77
hunmonk CreditAttribution: hunmonk commentedjust 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
Comment #78
dwwWhile 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
Comment #79
sun50 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.
Comment #80
dwwThanks 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"...
Comment #81
dww#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...
Comment #82
sun1) s/\$query/$result/
2) For ANSI SQL compatibility, there should be an AS between COUNT(nid) and project_count.
$uid = db_result($query)
(No need to create objects of type stdClass here)
Did we rename this table from _user to _global?
Makes less sense to me. :P
I wonder whether this kind of update string 1) really needs to account for singular/plural forms, and 2) be translatable and translated...?
Comment #83
dwwThanks for the review, sun! Up late, I see. ;)
Re: _user vs. _global:
http://drupal.org/commitlog/commit/1894/7cd94c9745a1f9838f686c82173d7bf8...
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.
Comment #84
dwwSweet! Just tested this on http://subscribe-drupal.redesign.devdrupal.org:
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!
Comment #85
sunAwesome!!! This is amazing - thanks for all your work on this @hunmonk + @dww!
Comment #86
dwwNow 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
Comment #87
sunLet's use a simple #type 'item' here; i.e., just change the #type and remove #collapsible.
It looks like we're not validating yet whether the new project is in the table already?
This should use form_set_value() instead.
Comment #88
dwwThanks 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...
Comment #89
sun"(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)"
Comment #90
Bojhan CreditAttribution: Bojhan commentedI 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.
Comment #91
pillarsdotnet CreditAttribution: pillarsdotnet commented@#89 by sun on October 8, 2011 at 7:37pm:
I had exactly the same confusion.
Comment #92
Bojhan CreditAttribution: Bojhan commentedused unless customized?
Comment #93
Lars Toomre CreditAttribution: Lars Toomre commentedI would suggest '(applies unless customized below)'. I agree with comments in #91 and #89 about confusion.
Comment #94
pillarsdotnet CreditAttribution: pillarsdotnet commentedI would be okay with
(applies unless customized below)
. It still sounds somewhat awkward to my mind, but I can't think of anything better.Comment #95
yoroy CreditAttribution: yoroy commented'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.
Comment #96
dwwI'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.
Comment #97
pillarsdotnet CreditAttribution: pillarsdotnet commentedHow about 'global default' or 'side-wide default' as distinguished from the per-project settings which follow?
Comment #98
dwwEither "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. ;)
Comment #99
dwwAfter 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...