[D7] Election Livestats

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

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.

I can't clone your module. It is asking password for clone the module.
Kindly check the git permission and set proper permission to access to clone the git by anonymous user.

1. Write properly the git details in project applications issues
example :

Project git : git clone --branch 7.x-1.x    http://git.drupal.org/sandbox/pathirakaliappan/2301783.git page_guide   
          cd page_guide

@leopathu, just a reminder - every user is able to edit any issue. I updated the clone command.

Note that review comments do not all need to be addressed in order for a project to pass review. Simply set the issue back to ‘Needs review’, whenever you have addressed everything that you think is reasonable.

My review disclaimers

• I am the author/maintainer of the Election module, and gausie was an colleague for some time, so you could consider me biased (I don't know how that works for reviews)
• I actually haven’t got the live stats working yet - but it’s not the module’s fault (I think). I need to figure out what’s happened to my Redis connection.

General comments

• The code shows an extensive knowledge of good Drupal practices, and good coding practices in general.
• There is not a lot of documentation (perhaps implementing hook_help() would be nice), but then again the Election module doesn't have much either, and this has a good README at least.

README.md file

This is impressively clear. Just a few notes:

• “Install Drupal module and configure” in the README.md is probably redundant, or at least you need to specify what step(s) I might need to take for configuration on the Drupal side.
• You should list the Highcharts library in the “Software requirements” section.
• Are there version requirements for phpredis, nodejs, or npm? … presumably. It’d be nice to list them if you can find them out. Likewise for the Election and Libraries modules.
• I’d suggest instead of “Start the ‘app.js file’” - “Run the app.js file (e.g. ‘node app.js’)”
• The security consideration seems scary. What can be done about that? I’d suggest there should be a corresponding warning in the description of the Drupal permission (the ‘View live statistics for an election’ permission).

Installation experience (so far)

• You hard-code the Redis host (on the Drupal side). This is what has defeated me so far, I expect. Can this be made configurable? I'd suggest a Drupal variable - variable_get('election_livestats_redis_host', '127.0.0.1')
• Can you modify app.js so that the ‘ssl’ key is not required? I had to like generate my own certificate (which meant I had to look up the right command on stackoverflow again).
• Is there a way to make the ‘npm install’ any faster? (is there a lock file you can commit or something, like Composer, or does npm not work that way) Repeatedly building this (on Platform.sh :)) is a little bit painful.

(Manual, IDE-assisted) code review comments

• There’s a code comment - election_livestats.module line 353 - which mentions UCLU, you probably don’t want that.
• You have some JavaScript objects with trailing commas, those may break something (IE?). Perhaps do a bit of jshint/jslinting.
• You declare a global 'key' variable at the end of app.js - is that intentional?
• The form builder function in election_livestats.actions.inc refers to a form submit callback - election_livestats_form_cache_eligibility() - defined in election_livestats.module. You may want to define the submit callback in the ...actions.inc file. In which case you should call form_load_include() inside the form builder so that ...actions.inc will always be loaded when the form is processed.
• You use an (as yet) nonexistent function election_vote_access_test(). Document that a patch is required? Or I should just commit the patch...

Just an update: I got around the Redis problem, and I posted a dubious patch in #2449911: Read config.json in the module to get the Redis host and port.

My next problem was with my hosting choice (it runs nodejs but won't let me expose the port, or daemonize it), so I'm going to see if I can test locally.

Thanks for this module. I had a quick look and noticed the following:

election_livestats.admin.inc file
At the end of your code there is an extra semicolon (;) on line 37 in hook_form().

   return system_settings_form($form);

};

README.md file
1) please correct the spelling mistake:
compatable ==> compatible

2) ensure line 29 has max of 80 characters as recommended by http://pareview.sh/pareview/httpgitdrupalorgsandboxgausie2446607git

Clone Repository
When I cloned this module, it cloned into a directory named '2446607'.
Please add the module name to the clone command so that it looks like this:

git clone --branch 7.x-1.x http://git.drupal.org/sandbox/gausie/2446607.git election_livestats

You can find the correct git clone command for your sandbox by clicking on the Version control tab, removing the checkbox in front of "Maintainer", and clicking Show. You can then copy-paste the git clone command from the codeblock below "Setting up repository for the first time".

Setting this to needs work.

Anyone can fix the clone command and the rest are surely not application blockers. Anything else that you found or should this be RTBC instead?

@gausie and @klausi I'm sorry for any inconvenience; now I know such minor issues should not delay the application.

Other than the small issues I mentioned, I was very impressed with the efficient and professionally written code.

I think this should be RTBC.

@pjcdawkins based on your review and mine do you agree this should be RTBC?

@drupalove I do, yes. There were thorough responses to comments and as you say the code shows the appropriate knowledge + skills.

@gausie it's not important, but the form_load_include() just means you can put your submit callback in the same file as the form function. The best description is in the docs: https://api.drupal.org/api/drupal/includes%21form.inc/function/form_load...

@gausie here's the workflow: https://www.drupal.org/node/532400

When a reviewer is confident that the application can be approved, a status change to Reviewed & tested by the community will inform the Code Review Administrators who will do a final review and set the status to Fixed upon approval, or bump it back to Needs work if there are still changes required.

We are currently quite busy with all the project applications and I can only review projects with a review bonus. Please help me reviewing and I'll take a look at your project right away :-)

Bumping... but @gausie I think it's a matter of policy that you'll have to go through the review bonus process (i.e. review 3 other applications and link the evidence from this issue) for this to proceed.

Thanks for your contribution!

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.