Currently the module adds some help text about setting the default value to the comment form.

Instead it should show that text after a user has posted a comment if they don't already have a default set.

Credit to alpritt for the idea.

CommentFileSizeAuthor
#22 16-02-2009 01-54-32.png3.27 KBneochief
#18 319830_simplified_interface_18.patch8.41 KBgreggles
#17 comment_notify.css_.txt228 bytesgreggles
#17 319830_simplified_interface_17.patch8.57 KBgreggles
#17 comment_notify.js_.txt539 bytesgreggles
#15 319830_simplified_interface_14.patch8.23 KBgreggles
#15 Picture 1.png13.02 KBgreggles
#15 comment_notify.css_.txt228 bytesgreggles
#13 pastedGraphic.png12.73 KBgreggles
#3 319830_settings_to_dsm.d5.patch2.95 KBaclight
#1 319830_settings_to_dsm.txt3.11 KBgreggles

Comments

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented
Assigned: Unassigned » greggles
Status: Active » Needs review
StatusFileSize
new 319830_settings_to_dsm.txt3.11 KB

Here's a patch which also fixes some whitespace issues and a notice.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented
Status: Needs review » Patch (to be ported)

Applied to 6.x-1.x.

aclight’s picture

aclight commented
Version: 7.x-1.x-dev » 5.x-2.x-dev
Status: Patch (to be ported) » Needs review
StatusFileSize
new 319830_settings_to_dsm.d5.patch2.95 KB

Here's a direct port for D5.

However, I'm -1 on the change this patch makes.

First of all, the message is "You can change the default for this field in...". What field? As a user, I see this message and will have no idea what the message means.

Second of all, this message is displayed each time the user posts a comment. If I was able to figure out what the message meant the first time, I probably don't need it to be displayed to me every time I post a comment on that site.

But if you want to keep the two branches in sync this patch should do it.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented

@aclight - great points. First, I think it is only shown if the user does not have a default setting"
if (empty($user->comment_notify_mailalert) && $comment['notify']) {

Won't the empty() test ensure it only gets shown until they set a default?

That said, I totally agree that the message should be changed. How about:

You can change the default for this field ...

to become:

You chose to receive notifications to comments on this post. To set a default notification go to...

Thoughts?

aclight’s picture

aclight commented

Yeah, it looks like it should only be displayed if no default has been chosen, but why pester a user over and over to set a default in the first place? I'd be fine doing this the first time a user posts a comment, but after that never giving the message, but doing this would require storing whether or not the user had been shown the message before, which complicates the module unnecessarily.

The original issue doesn't explain why this change was suggested/made in the first place, but I'm guessing it was to simplify that form element. However, I really don't think that the usability is improved by removing information about a form element and then displaying a message at a later time.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented

This was on the advice of alpritt in http://groups.drupal.org/node/15456 from the usability group.

It's certainly debatable whether it should be on the form or after submission. Generally speaking Drupal follows a pattern of telling users about the success/failure of an action that they have just performed "Your blog %title was created" for example. I think it makes sense to say something to the effect of "You will get comment notifications - you can make this your default" both to limit the complexity of the form and to follow the pattern of other parts of core.

aclight’s picture

aclight commented

I'd actually rather see this done using radios/checkboxes, but I see there's already an issue for this at #320224: Use check box to subscribe if only one subscription mode is available, radios otherwise, near the mailbox. I'll post a follow up there.

I see the point about how a user might click on a "make this my default" link on the comment form itself, and lose the comment. But I still really dislike excessive use of dsm().

Probably continuing discussion at #320224: Use check box to subscribe if only one subscription mode is available, radios otherwise, near the mailbox for now is best.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented
Priority: Normal » Critical

I was torn about this, but I think I've now got an idea.

My main problem with this is that I still wanted control over this in my account area and didn't want the form to override my selection.

Solution: it should only set the default for a user if there is no default :)

What do you think?

Also, I think that this issue needs to be solved before the next release so I'm bumping the priority.

aclight’s picture

aclight commented

I think that behavior would be fine. But with regards to my mockup in #320224-4: Use check box to subscribe if only one subscription mode is available, radios otherwise, near the mailbox (http://drupal.org/files/issues/comment_notify_comment_settings.png), the text would need to change, presumably to something mentioning the possibility of setting the per-user default, which then brings us back to the same problem of having a link that might take the user away from the form they are filling out.

I agree that this would be a good thing to get fixed before the next release.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented

So...I'm debating about how to implement this change.

In order to do this right now (because comment_notify saves data in $user->data) we would have to do a user_save which I'm not entirely comfortable doing...

What I'd rather do is insert a record in a single little table with UID and preference. Updating to that seems like a good thing to do regardless - #348000: stop using the $user->data to store preferences.

aclight’s picture

aclight commented
Status: Needs review » Needs work

Patch in #3:

apfel-ethernet /drupal/iex/dev/sites/all/modules/comment_notify: patch -p0 --dry-run < 319830_settings_to_dsm.d5.patch 
patching file comment_notify.module
Hunk #2 FAILED at 138.
Hunk #3 succeeded at 257 (offset -10 lines).
Hunk #4 succeeded at 350 (offset -10 lines).
Hunk #5 succeeded at 374 (offset -10 lines).
1 out of 5 hunks FAILED -- saving rejects to file comment_notify.module.rej

Setting back to CNW

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented
Title: move message about changing the settings to a dsm after submission » create a simplified user interface

Changing title to reflect comments 6 through 9.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented
StatusFileSize
new pastedGraphic.png12.73 KB

Noyz suggested to me

Both seem adequate. I like the later (http://drupal.org/node/360013) because it requires less real-estate and feels simpler, but I think the former (http://drupal.org/files/issues/comment_notify_comment_settings.png) is more usable because it's more obvious. The later is what's currently being used, and I failed to see it.

If it were my design, I'd probably go with the former - with a couple tweaks.
- loose the help text (your comment settings will be remembered. Drupal has too much stuff on the screen, and I don't think this is enough of a value add to warrant it. On the next visit, they'll figure this out.
- change the statement to: Notify me when new comments are posted.

I like his solution. Thoughts?

aclight’s picture

aclight commented

Looks good to me

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented
Version: 5.x-2.x-dev » 7.x-1.x-dev
StatusFileSize
new comment_notify.css_.txt228 bytes
new Picture 1.png13.02 KB
new 319830_simplified_interface_14.patch8.23 KB

And here's a patch for 6.x... (and screenshot, but it's pretty right on).

Also note the new .css file. I'm not super happy with how I clear the float, but I tried a few other things and they don't work. I figure a real css guru will provide advice soon enough.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented
Status: Needs work » Needs review
greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented
StatusFileSize
new comment_notify.js_.txt539 bytes
new 319830_simplified_interface_17.patch8.57 KB
new comment_notify.css_.txt228 bytes

Let's try this again...

Now with 100% more javascript. If the checkbox is blank and a user clicks on the radio button then the JS automatically clicks the "notify me" checkbox.

This also refactors the form_alter to remove my copy paste anti-pattern. Now there is an if condition to test whether the user is anonymous or not and only the unique code is inside the if instead of duplicating code. Should remove a bug (unreported, i think) where the registered user default would be used for anonymous.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented
Version: 7.x-1.x-dev » 5.x-2.x-dev
Status: Needs review » Patch (to be ported)
StatusFileSize
new 319830_simplified_interface_18.patch8.41 KB

Ok, tested this out a bit more and committed it. Now to more fun stuff.

http://drupal.org/cvs?commit=171577

I'll leave this in "to be ported" state for a while. I don't plan to port it myself, but if someone else cares about 5.x then they can do it... (an initial port is attached).

dragonwize’s picture

dragonwize commented

EDITED: Sorry, I was testing without the latest update. Seems to be working fine now. Please disregard this.

Small bug with the 6.x patch.

case 'load':
      $user_settings = db_fetch_array(db_query('SELECT node_notify AS node_notify_mailalert, comment_notify AS comment_notify_mailalert FROM {comment_notify_user_settings} WHERE uid = %d', $user->uid));
      foreach ($user_settings as $property => $value) {
        $user->$property = $value;
      }
      break;

This causes a "warning: Invalid argument supplied for foreach()" error if the user does not have any comments to be notified of in the the table. Need an if wrapped around the foreach.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented

Thanks dragonwize! I found/fixed that prior to committing and have now turned on notices in my dev environment.

neochief’s picture

neochief commented

There are two variables that should be the same in a current 6x-dev version:
comment_notify_default_regged_mailalert
comment_notify_default_registered_mailalert

neochief’s picture

neochief commented
StatusFileSize
new 16-02-2009 01-54-32.png3.27 KB

Also, it would be nice to add aditional style to place some whitespace between the radiobutons:

#edit-notify-type-2-wrapper {
margin-left: 1em;
}

because under windows, we have such a bad picture:

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented

@neochief - I've committed exactly what you provided to the 6.x branch. Seemed good to me.

neochief’s picture

neochief commented

You should also get rid of double variables, as I mentioned in my previous comment (#21). They both present in dev. version currently. Should be only one of them.

(Sorry, I have no ability to make patch at this time by myself)

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented

Yes, but that should be a separate issue. #375378: duplicated variables

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented
Assigned: greggles » Unassigned

(I'm not working on porting...)

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented
Status: Patch (to be ported) » Closed (fixed)

This module is no longer actively developed for Drupal 5. If someone wishes to take over as the 5.x maintainer I would consider it, but these days everyone should really upgrade to Drupal 6.x.