Maestro Toolbar Notifications

Hi Calvin,

I didn't run it through Coder module, but nothing really sticks out as a problem in the module code. The main thing is in the Javascript. You should be using Drupal.behaviors rather than the usual $(document).ready(){} syntax.

See:
http://www.drupal4hu.com/node/314 and
http://drupal.org/node/756722

Looks goo to me. I don't have any other suggestions. I'd suggest getting another set of eyes on it (preferably one of the Maestro maintainers) and then it should be good to go.

Edit:

There's some minor formatting errors. Just found the new review tool that people are using now... It wasn't around last time I reviewed a project application.

http://ventral.org/pareview/httpgitdrupalorgsandboxcalvintennant1550934g...

Minor styling issues are no reason for setting needs work, please consider doing a in-depth review.

Good Work!

There are still files other than README.txt in the master branch, make sure to remove them. See also step 5 in http://drupal.org/node/1127732.

My only observation is a little change in your comment, Drupal instead Drapl

* Adds a Maestro notification to the Druapl toolbar

Again, please don't switch to needs work on minor issues, this just block deep reviews.
Set to RTBC if you think it's okay.

Patrick, if you think it's OK not to block on minor formatting, then I'm OK to RTBC. I'm more familiar with maybe a year or so ago when things were more nitpicky.

+1 RTBC - go ahead soncco and change the status if you're happy with the module.

Hi all

Is it good practice to call the function maestro_toolbar_notifications_count() without using a hook ?

Cheers,

I have done an in-depth code review on this project, and have the following comments to be addressed:

1. Automated code review found a few errors:
http://ventral.org/pareview/httpgitdrupalorgsandboxcalvintennant1550934git

2. It is not a good idea to call maestro_toolbar_notifications_count() as a global function the way you do even if you need it called on every page load. This is a huge performance hit. Remember that even AJAX functions will run this, even if they don't need to. Because it's a global function, even things like image style generation could run it. Further, this looks like something that only needs to be run under certain conditions (authenticated users, users with certain permissions, etc.)

A better approach would be to hook_preprocess_page() and/or hook_init() if absolutely necessary, and also to wrap the check in some sort of user-access filter before doing the actual counting work to minimize workload in conditions where it's not required. (For most sites, 90%+ of workload is anonymous).

3. When running a db_query to get just a count, the results are not used by the function. This is inefficient. db_query provides a rowCount() function you could call instead of foreach()ing results you don't actually use, and/or you could modify the query to perform a count(*) instead of a select(*). MySQL can cache this type of query very well so it is MUCH faster.

4. You are forcing the site to load the JS file for this tool whether it's actually required or not. You also expose the maestro_toolbar_notifications setting for all users as well. The security risk here is pretty minimal, but as a rule data should not be exposed unless it is actually necessary. Information about the presence of a module makes automated vulnerability scanning/exploit work easier. If "maestro taskconsole" is required to use the notification, the setting and JS should not be included unless the user has this permission. A simple check_access() at the top of maestro_toolbar_notifications_count() would handle this easily.

5. In the JS, you replace #toolbar-link-admin-notifications with a new value without checking to be sure the value is defined. Please wrap this in a
typeof(Drupal.settings.maestro.....notification) !== 'undefined'
otherwise it is possible to get a JS error that stops other items from processing in the event the setting is not properly defined, or something happens to it.

6. This is a small module. I'm a fan of small+simple, but the Drupal community has established a rule that new project submissions must be at least 120 lines - this is about half that. This is to illustrate the author's competence in the first project submission (and maybe to see if code reviewers are being honest as well?) Anyway, this module needs to be longer. Some suggestions:

a. Add a hook_help (http://api.drupal.org/api/drupal/modules!help!help.api.php/function/hook...).
b. Make the changes in #1-#5. It will probably take 30 lines of code to address them all.
c. Add tooltip text or even a drop-down on the notifications display item that summarizes the notifications awaiting the user.

Please note: I am not saying this module is not useful as-is. It definitely is (for Maestro users, anyway). But this is the policy, so I am just making suggestions on easy/quick ways to meet the requirement.

I have reviewed the above-mentioned changes, and found nothing new to report. Moving to RTBC.

Your project page is not very informative you may have a look at the tips for a great project page.

I'm afraid that this project is too short to approve you as git vetted user. We are currently discussing how much code we need, but everything with less than 120 lines of code or less than 5 functions cannot be seriously reviewed (we can't make sure that you can code securely/correctly if there is not enough code to review). However, I can promote this project manually. Sorry, I hope you can understand that.

Thanks for your contribution!

I've promoted this module to a full project and now your able to create releases.

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.

As you continue to work on your module, keep in mind: Commit messages - providing history and credit and Release naming conventions.

Thanks to the dedicated reviewer(s) as well.

The project Maestro Toolbar Notifications has been promoted to a full project.
New URL

Now that this experimental project has been promoted, you'll need to update the URL of your remote repository or reclone it.

git remote set-url origin YOURUSERNAME@git.drupal.org:project/maestro_toolbar_notifications.git