[D7] Quiz Result Export

Git clone command for the sandbox is missing in the issue summary, please add it.

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.

@Shaggy

To help others, the title should include the Drupal version. Ex: [D7] Quiz Result Export per https://www.drupal.org/node/1011698.

The link to the Sandbox project is a 404 (http://git.drupal.org/sandbox/Shaggy/2532144.git). It should be https://www.drupal.org/sandbox/shaggy/2532144.

The git clone instructions on https://www.drupal.org/node/2532144/git-instructions/7.x-1.x/nonmaintainer are different from what you have since you are the maintainer. Updating it to git clone --branch 7.x-1.x http://git.drupal.org/sandbox/Shaggy/2532144.git quiz_result_export would help others clone the repo quickly.

Automated Review

Looks good. No automated test cases were found, but is not a requirement.

Note that perfect adherence to Drupal Coding Standard is NOT a reason to block an application, except for total disregard of them. However, modules should follow them as closely as possible.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

No: Causes module duplication and/or fragmentation. https://www.drupal.org/node/2521904 duplicates this functionality.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements.

3rd party assets/code

Yes: Follows the guidelines for 3rd party assets/code.

README.txt/README.md

No: Does not follow the guidelines for in-project documentation and/or the README Template. The project description and readme.txt need more text.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes: Meets the security requirements.

Coding style & Drupal API usage

[List of identified issues in no particular order. Use (*) and (+) to indicate an issue importance. Replace the text below by the issues themselves:

1. It would be nice if there were more inline comments. After line 79 there aren't any comments for the rest of the file. So 45 lines I'm not too sure what it does including the foreach loop.

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

This review uses the Project Application Review Template.

@shaggy

Automated Review
Looks Good .

Manual Review

Individual user account
Yes: Follows the guidelines for individual user accounts.

No duplication
https://www.drupal.org/node/2521904 , implements the same functionality.
Please consider mentioning the reason or any further enhancement you are plannning to justify.

Master Branch
Yes: Follows the guidelines for master branch.

README.txt/README.md
No: Remove .README.txt.swp file from sandbox.

Licensing
Yes: Follows the licensing requirements.

3rd party assets/code
Yes: Follows the guidelines for 3rd party assets/code.

Coding style & Drupal API usage
The best practice is to use Drupal functions rather than using their PHP counter parts.
* Use drupal_add_http_header insead of header()

@Shaggy

Individual user account
Yes: Follows the guidelines for individual user accounts.
No duplication
No: Causes module duplication and/or fragmentation. https://www.drupal.org/node/2521904 duplicates this functionality.
Master Branch
Yes: Follows the guidelines for master branch.
Licensing
Yes: Follows the licensing requirements.
3rd party assets/code
Yes: Follows the guidelines for 3rd party assets/code.
README.txt/README.md
No: Does not follow the guidelines for in-project documentation and/or the README Template. The project description and readme.txt need more text.
Code long/complex enough for review
Yes: Follows the guidelines for project length and complexity.
Secure code
Yes: Meets the security requirements.
Coding style & Drupal API usage
[List of identified issues in no particular order. Use (*) and (+) to indicate an issue importance. Replace the text below by the issues themselves:

* Since die is equivalent to exit() and Drupal has a wrapper for that function; I'd recommend to use drupal_exit() instead of die. (Line 128)
* Comment for hook_permission says "Implements hook_perm()"; it should be "Implements hook_permission()".
* From lines 86 to 92; there are strings without t funcion. I think it'd recommend you to wrap those strings in t function.

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

Hi Shaggy,
The quiz results can be exported using Views Data Export module which is already mentioned on the Quiz project page. Why this module is required anymore?

Module duplication is not an application blocker anymore, we can only point applicants to existing solutions but it is their decision to use that or not.

Hi,

In this project Please remove the core parameter form the quiz_result_export.info because drupal.org will add the core parameter in project when the project will error free and going to be a full project.

Automated Review

Lines 5, 6 and 24 in README.txt significantly exceed 80 characters and should be manually wrapped

Note that perfect adherence to Drupal Coding Standard is NOT a reason to block an application, except for total disregard of them. However, modules should follow them as closely as possible.

Manual Review

This module is compatible with the 7.x-4.x version of the quiz module and not with version 7.x-5.x. As version 5 is recommended for new installs it would be nice if a note was made of this on the module page, or even better if the module was made compatible with both versions.

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

No: Causes module duplication and/or fragmentation. As mentioned above there are other methods to export quiz results to csv. Good handbook documentation on how to do this would be better than a separate module. This is not an application blocker, although reference to the other methods should be made on the module page.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements.

3rd party assets/code

Yes: Follows the guidelines for 3rd party assets/code.

README.txt/README.md

Yes: Follows the guidelines for in-project documentation and/or the README Template (with the exception of line lengths noted above).

Code long/complex enough for review

Probably does not follow the guidelines for project length and complexity. This module only just reaches the length requirements and only contains four function definitions. A git admin could choose to promote this one project without granting the git vetted user role however.

Secure code

Yes: Meets the security requirements.

Coding style & Drupal API usage

1. There is a capitalization error in the permission description. The description should also be reworded for clarity.
2. There is no need to give a description for a MENU_CALLBACK menu item, although again there is a capitalization error and the description is again unclear.
3. Consider using %node in the menu path to invoke node_load and pass the node object rather than the node id to quiz_result_export_get_file().
4. Consider using an alternative delivery callback rather than mixing the code for creating a file download with the logic of exporting the data in the same function.
5. Consider a more specific form hook rather than implementing the generic hook_form_alter and checking the form_id.
6. Consider moving quiz_result_export_get_file() out of the .module file and into a separate .inc file to reduce the amount of code loaded on each page request.

These comments in the code walkthrough are all recommendations and are not application blockers.

This review uses the Project Application Review Template.

(forgot to update status)

@mdryan: thanks, that are useful suggestions for improving the module. They are not critical application blockers though, anything else that you found or should this be RTBC instead?

Agree, nothing that's an application blocker.

Thanks for your contribution, Shagun!

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.