[D7] SN_BAZAARVOICE

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

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.

Hi,

In sn_bazaarvoice.install:

• you don't longer need drupal_install_schema('sn_bazaarvoice') and drupal_uninstall_schema('sn_bazaarvoice') - read here https://api.drupal.org/api/drupal/includes!common.inc/function/drupal_in...
• you should delete your variables in sn_bazaarvoice_uninstall() like this:

variable_del('sn_bazaarvoice_bzrvoice_api_version');

in sn_bazaarvoice.module:
sn_bazaarvoice_configure() ... you are calling snBazaarvoice_BazaarvoiceAdmin class, but in admin.inc you have snBazaarvoice_snBazaarvoice_BazaarvoiceAdmin class, you should fix that.

Multiple Applications

It appears that there have been multiple project applications opened under your username:

Project 1: https://drupal.org/node/2035493

Project 2: https://drupal.org/node/2034513

As successful completion of the project application process results in the applicant being granted the 'Create Full Projects' permission, there is no need to take multiple applications through the process. Once the first application has been successfully approved, then the applicant can promote other projects without review. Because of this, posting multiple applications is not necessary, and results in additional workload for reviewers ... which in turn results in longer wait times for everyone in the queue. With this in mind, your secondary applications have been marked as 'closed(duplicate)', with only one application left open (chosen at random).

If you prefer that we proceed through this review process with a different application than the one which was left open, then feel free to close the 'open' application as a duplicate, and re-open one of the project applications which had been closed.

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

Hi !
=> Manual review on your "sn_bazaarvoice_dev" branch...
* First I'm not sure at all about your latest branching name : "sn_bazaarvoice_dev" and since Pareview.sh is also complaining about that, I would suggest to use versionning standard name instead. I mean like : 7.x.1.x

* I believe your hook_schema on .install could set some field (like nid) as "unique" key and/or indexed.

* You have an empty folder "sn_bazaarvoice"... If it is used to store file (I don't think so), I would suggest using file directory and file api instead.

* Your js file rating.js look quite odd compared to Drupal javascript standard. Why not using Drupal.behaviors instead ?

* You can remove 'type' => MENU_NORMAL_ITEM, in your hook_menu as it is default.

The rest is not looking strange to my 'non-expert' eyes.
Best !
++

Drupal.org has the following policy:

All user accounts are for individuals. Accounts created for more than one user or those using anonymous mail services will be blocked when discovered.

Can you confirm that the sn_idea_engineers account is a single user? Filling out your profile would help.

If you prefer, we can also promote this sandbox to a full project without granting you "git vetted user" access.

----
Top Shelf Modules - Enterprise modules from the community for the community.

Hello

I just reviewed your new work :
* Branching name : I'm not sure that 7.x-1.0-dev naming is indicating a branch at all. It does look like a release name.
I beleve 7.x-1.x is a branch while 7.x-1.0-dev is a dev release.
* I just found db_select() l.90 of your code. Why not using the faster db_query instead if your query is not dynamic ?
Also please use correct syntax for this kind of queries :

db_select('my_table')
  ->conditions('bla', $bla)
  ->conditions('blabla', $blabla)
  ->execute();

is easyer to read than :

db_select('my_table')->conditions('bla', $bla)->conditions('blabla', $blabla)->execute();

* l.93 I guess the proper way of using this logic is by an if / else statement instead of multiple if ?
* l.180 please don't add a blank line beetween your if / else statement.
* Maybe doc comment are also needed on CSS file. I would add it.

Except thoses points (sorry for having added a few afterward), everything is looking good for me now !

Ok.
It's RTBC for me now.
Changing the status to "reviewed and tested by the community".
Thank for getting involved.
++

Hi,

I set your project as reviewed and tested by the community.
I'm not a member of the review team and cannot promote your account myself. Basically it's the maximum I can do.
You're now waiting such a member to come and give a deeper review to your project (they do have better eyes than mine).
Once they will be pleased by your work, they will promotte your account.

As you can see, they're VERY busy with a lot of project to review.
To faster the process, you can consider applying for a Review bonus by helping them reviewing others projects !
3 review needed. Read more => https://drupal.org/node/1975228
I used it (twice) for my own project and got vetted in 4 days.
Cheers !

I tested the functionality. its working fine.

Don't forget to add the "PAReview: review bonus" tag as indicated in http://drupal.org/node/1975228 , otherwise you won't show up on my high priority list.

manual reivew:

1. project page is too short, see https://drupal.org/node/997024 What is that service? What can be reviewed? How does it work?
2. sn_bazaarvoice_init(): why do you need your CSS/JS on every single page request? You should only add it when you display your review stuff.
3. sn_bazaarvoice_submit(): doc block is wrong, this is not a hook? See https://drupal.org/node/1354#functions under page callbacks.
4. Why do you use sn_bazaarvoice_submit() as form submit function and page callback? That is very confusing.
5. sn_bazaarvoice_install() and sn_bazaarvoice_uninstall() should be removed, Drupal 7 will install the schema automatically.
6. sn_bazaarvoice_theme(): all templates and theme keys should be prefixed with your module's name to avoid name collisions with others.
7. reviews-list.tpl.php: all user facing text must run through t() for translation.
8. sn_bazaarvoice_preprocess_reviews_list(): this is vulnerable to XSS exploits. The review data stems from an untrusted third party source and needs to be sanitized before printing. Please read https://drupal.org/node/28984 again. And please don't remove the security tag, we keep that for statistics and to show examples of security problems.
9. sn_bazaarvoice_preprocess_review_box(): You are not doing and preprocessing of variables here? The form should be created directly in sn_bazaarvoice_node_view().

Review bonus tag is already removed, you can add it again if you have done another 3 reviews of other projects.

Multiple Applications

It appears that there have been multiple project applications opened under your username:

Project 1: https://drupal.org/node/2035493

Project 2: https://drupal.org/node/2034513

As successful completion of the project application process results in the applicant being granted the 'Create Full Projects' permission, there is no need to take multiple applications through the process. Once the first application has been successfully approved, then the applicant can promote other projects without review. Because of this, posting multiple applications is not necessary, and results in additional workload for reviewers ... which in turn results in longer wait times for everyone in the queue. With this in mind, your secondary applications have been marked as 'closed(duplicate)', with only one application left open (chosen at random).

If you prefer that we proceed through this review process with a different application than the one which was left open, then feel free to close the 'open' application as a duplicate, and re-open one of the project applications which had been closed.

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

Please note that organization accounts cannot be approved for git commit access. See https://drupal.org/node/1966218 and https://drupal.org/node/1863498 for details on what is/isn't allowed. Please update your user profile so that we don't have to assume that this is a group account.

Don't forget to add the "PAReview: review bonus" tag as indicated in http://drupal.org/node/1975228 , otherwise you won't show up on my high priority list.

I'll look at this now in the Project applications sprint

Review of the 7.x-1.x branch:

• DrupalPractice has found some issues with your code, but could be false positives.
  

  
  FILE: /home/klausi/pareview_temp/rating.inc
  --------------------------------------------------------------------------------
  FOUND 0 ERROR(S) AND 2 WARNING(S) AFFECTING 2 LINE(S)
  --------------------------------------------------------------------------------
    19 | WARNING | Unused variable $userid.
   215 | WARNING | Unused variable $userid.
  --------------------------------------------------------------------------------
  

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.

manual review:

1. sn_bazaarvoice_init(): this is still here - why do you need the JS setttings on every single page request? Why can't you add them in sn_bazaarvoice_node_view()?
2. SnBazaarvoiceAdmin: why do you have a class that only has static methods? You could as well just use functions instead?
3. admin.inc: all variables defined by your module need to be removed in hook_uninstall().
4. sn_bazaarvoice_node_view(): module_load_include() is called twice?
5. sn_bazaarvoice_node_view(): don not call render() or drupal_render() here, Drupal core will generate markup from the render array later automatically.
6. sn_bazaarvoice_preprocess_sn_bazaarvoice_reviews_list(): do not use arg() here, if you need the node ID you should pass it into the theme function.
7. sn_bazaarvoice_node_view(): instead of calling drupal_add_css/js() I would recommend to use #attached on the render array, so that it could be cache as whole.

Not absolute critical blockers, so this looks RTBC to me. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

I'll look at this now in the Project applications sprint.

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

• You have a couple of unused variables reported here: http://pareview.sh/pareview/httpgitdrupalorgsandboxsnideaengineers202745...
• Your README has duplicated text?
• The comment in hook_schema() // We should not use ) here. doesn't make sense.
• In sn_bazaarvoice_node_view() it would be nice if you only loaded the js/css *after* you check the node type to see if its enabled for the rating widget.
• Why are you using @render? Could you use some other method to determine if there's a problem with the render?
• In SnBazaarvoiceAdmin::getConfigurationForm() you have a typo, "Provie" should be "Provide".
• Use drupal_strlen() instead of strlen(), which is not multibyte safe.
• In sn_bazaarvoice_fetch_reviews() why do you set up $params and use drupal_http_build_query() and then in the next line, you manually add Filter? I think it would be easier to just add that to the params array.
• plus all the stuff klausi said :)

All the module code commits seem to belong to Divesh Kumar though, is that who should be the git vetted user? We can still promote this project for you manually if you like.

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

You did not list any new reviews of other projects in the issue summary?

manual review:

1. sn_bazaarvoice_init() is still there, why?
2. sn_bazaarvoice_preprocesss_sn_bazaarvoice_review_box() is still there, why? Adding a form should not be done in a preprocess function?
3. Seems like you did not take a look at all of my previous review comments?
4. sn_bazaarvoice_getConfigurationForm(): functions should use only lower case characters and under scores. Also elsewhere.

Otherwise I think this is ready. Could you change your username to something singular so that it does not appear to be a group account?

Thanks for your contribution, sn_idea_engineers!

I updated your account so you can 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:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and stay 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.