Flag Notify is a simple but flexible module that uses the great flag module for email notifications of site activity and optionally integrates with organic groups.

It uses a hierarchical notification system organised in three levels: groups,nodes,comments, where users are always notified only once for the same event, with preference to the most relevant for them (i.e. a reply to own comment is notified as such rather than "there is a comment in your group").
Any these three "levels" are optional and if the Organic Group module is not installed the group level is just hidden.

Flag Notify purposely do not build the flags in code (it just ask for the flag machine name), nor it creates views of elements that have been subscribed.
While this approach require little bit extra work from the users, it increases the flexibility and avoid potential problems if the og module is enabled/disabled after this module.
If necessary, I can provide an export of flags and/or views.

Default notification for users are managed trough an option field in user profile so they can can choose their default notification settings when they submit a comment, a node or join a group.

Currently the notification email text is fixed (with a few customisations as footer and salutation). A token-based email may be developed in the next versions if there is enough request for that.

While a basic notification system could be obtained with just rules and flags, it would be very unhandy when you consider that a single event could trigger multiple and different kind of notifications and you have to choose between them.
An example is the post of a comment that could trigger both "There is a reply to your comment" and "There is a comment on your group", where you want your user to be notified only once, with an email template that match the specific case.
Further, it would be hard using just rules to retrieve the people that flagged all the parent comments in order to notify them.

Links

My own reviews (of other sandbox projects):

Comments

sylvaticus’s picture

sylvaticus commented
Issue summary: View changes

minor grammar typos

sylvaticus’s picture

sylvaticus commented
Issue summary: View changes

minor typo(2)

PA robot’s picture

PA robot commented
Status: Needs review » Needs work

There are some errors reported by automated review tools, did you already check them? See http://ventral.org/pareview/httpgitdrupalorgsandboxantonellolobianco2028...

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

Cristobal Wetzig’s picture

Cristobal Wetzig commented

Hey! So i looked trough your module, some things:

As you can see by the automatic controller, you have some standard issues in your code:

http://ventral.org/pareview/httpgitdrupalorgsandboxantonellolobianco2028...

Manual review

Grammar: You settings have some grammar to touch up. Examples:

"In the flag module create a "Receive notifications of this group's activity" flag and write here its machine name."

Should be something like. In the flag module, create a "...." and input the machine name here.

However! Should the user really look up the machinename for the flag? you should retrive available flags and let the user make the input by a #select type. You have no validation to check if the user made a correct input. What if the user by accident has an extra space character. If you only let users select the flag from a selectbox, you will never have that problem.

You are using dpm() to make dbug messages, but this might not be available for a users setup. is this only for development? use watchdog() function instead as this is certain to be available for any user.

the variable $fullthread = $comment->thread; is never used ? flag_notify_main()

Have a nice day!

sylvaticus’s picture

sylvaticus commented
Status: Needs work » Needs review

Flags as select list rather than text input

I fully agree with you, and I implemented the flag selection as select lists. I did try to define flags in code using the flag api, but there are some issues when the flag is cancelled trough the UI and more in general to keep in sync the module status (disabled/uninstalled/re-enabled..) with the flags status, so I prefer the approach of "user take care of creating the flags and we are only asking them which one to use".

Dpm

Yes, dpm() calls are never executed, but if I have a bug report they are on handy for me as I can call the main function with debug=TRUE and get valuable information.
For as far as I know using watchdog I can log messages but I can't store structured variables unless I serialize them.

Unused variable

Removed $fullthread, thank you!

Code Standards

0 Errors and 0 Warnings :-)

English spelling/grammar

It must be evident that English is not my native language :-|. Where could I ask for help in reviewing the spelling? Is there an "English group" in Drupal (I know of localisation groups..) ?

Thank you for your review!

sergeypavlenko’s picture

sergeypavlenko commented
Status: Needs review » Needs work

Hi

Apparently you updated code of the module, as new bugs are found on the page - http://ventral.org/pareview/httpgitdrupalorgsandboxantonellolobianco2028....

Also, you have a file in the repository "flag_notify.it.po" needs to be removed, the languages ​​are added separately through the service - http://localize.drupal.org/.

sylvaticus’s picture

sylvaticus commented
Status: Needs work » Needs review

Thanks, sorted. I don't know why, Code review run locally didn't saw the errors..

sylvaticus’s picture

sylvaticus commented
Issue summary: View changes

added review #1 of other sandbox projects

gabesullice’s picture

gabesullice
Primary language English
commented

@sylvaticus I really like what you're trying to do, but I wonder how this module differs extensively from Rules. Would you consider adding that clarification to your issue summary. If you send me a message when this is RTBC, I'll be happy to go through and make spelling/grammar corrections and get them to you as a patch.

sprice’s picture

sprice commented

It seems like many modules could be replaced by Rules, but many users also seem to like more "out of the box" solutions as this would provide. For newer site builders, rules can be a bit scary, and since this module doesn't depend on Rules, it can be used for more specific use-cases without needing something as powerful as Rules.

I don't think that there is a substantial overlap problem in this case.

I'm leaving the issues with the "needs review" status so that others can weigh in on this issue.

sylvaticus’s picture

sylvaticus commented

I will still write a point on this topic in the module description tomorrow, but the point is that being event-based a rules approach would be fairly complex to obtain without email duplication.
For example a new comment can triggle 3 notifications to the same user because it is at the same time a reply to one comment, a comment to his/her node and a comment in the same group.
To avoid multiple notifications I would have to cover all these scenarios in a single rule, but considering that i need to have three different type of email for the three scenarios and that i need to pick up the subscribers to all parent comments then it would be not trivial at all...

sylvaticus’s picture

sylvaticus commented
Issue summary: View changes

added a review item

sylvaticus’s picture

sylvaticus commented

I revised the English language usage (but still corrections are probably necessary and welcome!) and added a clarification on this thread description that highlights the "added value" of this module compared with just using flags+rules.

webengage_nitin’s picture

webengage_nitin commented

Hi,

1. please use "Implements hook_permission" for function: flag_notify_permission and leave one blank line after description.

2. In flag_notify_help(), you are using html entities inside t() function

3. use hook_perms for function: flag_notify_perms in description

Regards,
Nitin

sylvaticus’s picture

sylvaticus commented

1) done
2) done (more clear now, thanks!)
3) sorry, didn't get it! What do you mean?

Thank you and regards,
Antonello

webengage_nitin’s picture

webengage_nitin commented

Antonello:

I meant write description for the hook, similar to the first point. Nothing major, but helps to maintain Drupal standards :).

The code otherwise follows standards wondefully.

Regards,
Nitin

sylvaticus’s picture

sylvaticus commented

Just to be sure I understood correctly.. flag_notify_perms() is NOT an implementation of an hook like e.g. flag_notify_menu() is an implementation of hook_menu().

It is just a callback function defined in flag_notify_menu() (row 87 to be precise). I updated the description to have it described more clearly.

Cheers,
Antonello

kscheirer’s picture

kscheirer
Primary language English
Location Vallejo
commented
Status: Needs review » Needs work

You should remove the .gitignore file from the repo
You have a typoe in your readme, "INSTRUCTIIONS"
If flag_notify_perms() is not implementing a hook, can it be renamed? I'm not sure what effect that will have on the permissions page.
I'm not sure about leaving all the dpm() calls in there - doesn't that make this module depend on devel being installed?

Otherwise this looks good. Setting to needs work for the dpm/debugging issue. I'm not sure it's wrong, but what's your opinion?

----
Top Shelf Modules - Crafted, Curated, Contributed.

sylvaticus’s picture

sylvaticus commented
Status: Needs work » Needs review

Hello, sorry for the delay, I moved job and country...

That code was never executed, but it would have been on hand for me if users was going to report some problem on their site. Any how it was very short code so I can just reimplement it if needed.
I hence removed the debug code, fixed the typo and added a configuration option to suspend all notifications (on hand if the admin has to do some work on the site and he/she doesn't want to spam the users).

Setting back to needs review...

/Antonello

(ps: cc if you are pulling, as the menu callback function has been renamed)

kscheirer’s picture

kscheirer
Primary language English
Location Vallejo
commented
Title: [D7] - Flag Notify » [D7] Flag Notify
Status: Needs review » Reviewed & tested by the community

Looks good, thanks for those updates!

http://pareview.sh/pareview/httpgitdrupalorgsandboxantonellolobianco2028... says you have an unused variable, but that's very minor.

----
Top Shelf Modules - Crafted, Curated, Contributed.

sylvaticus’s picture

sylvaticus commented

Thank you, removed.

kscheirer’s picture

kscheirer
Primary language English
Location Vallejo
commented
Status: Reviewed & tested by the community » Fixed

It's been a month without any problems reported, so I'm promoting this myself as per https://drupal.org/node/1125818.

Thanks for your contribution, sylvaticus!

I updated your account to let you promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and get involved!

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

Thanks to the dedicated reviewer(s) as well.

----
Top Shelf Modules - Crafted, Curated, Contributed.

kscheirer’s picture

kscheirer
Primary language English
Location Vallejo
commented
Issue summary: View changes

Added "added value" compared with using rules.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.