[D7] Sign for acknowledgement

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and 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 Pagolo,

Nice module. Here's my manual review.

- Check out issues reported by parreview: http://ventral.org/pareview/httpgitdrupalorgsandboxpagolo1945276git
- It seems you're using italian names for form elements. I think it would be better to keep it English, so we can understand ;-)
- Maybe a little late, but I think you could use the field API to accomplish the same. Have you thought about that? It would be nice, because you could use the functionality in a much broader perspective.
- Is there a way to change the output of the checkbox & table? Would be nice to be able to use a template for this.
- You're showing a list of all users, using your own query. This way blocked users are also shown.
- The settings form has the option: First name field, as inserted in ACCOUNT SETTINGS->MANAGE FIELDS. Couldn't you present a dropdown list of possible fields?
- Have you thought about privacy? You're showing every user full names of other users including date and time when they read the document.
- There is no way to undo an acknowledgement, not even by an administrator.
- Acknowledgements are not being deleted when running hook_uninstall.
- Something to think about: when I have signed for acknowledgement, and the document is changed, it's still signed. I think the acknowledgement should be removed.

Overall I think the concept behind the module is great and I can see potential specifically in places with a complicated workflow. The thing I'm most concerned with, as beljaako pointed out and forgive me for saying it twice, is that after editing the document in question the signed check box is still checked. When the article in question is edited the box should become unchecked for all users who have already acknowledged it. This would prevent things like posted proposals being edited without the knowledge of users who have already read the document.

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

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards).
  

  
  FILE: /home/klausi/pareview_temp/sign_for_acknowledgement.module
  --------------------------------------------------------------------------------
  FOUND 3 ERROR(S) AFFECTING 3 LINE(S)
  --------------------------------------------------------------------------------
   368 | ERROR | Line indented incorrectly; expected 0 spaces, found 1
   373 | ERROR | Inline comments must end in full-stops, exclamation marks, or
       |       | question marks
   377 | ERROR | Closing brace indented incorrectly; expected 1 spaces, found 0
  --------------------------------------------------------------------------------
  
  Time: 1 second, Memory: 8.75Mb
  

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. sign_for_acknowledgement_form(): I think the checkbox #title string should be configurable.
2. sign_for_acknowledgement_form(): If a form has a submit function, then hidden form values are not needed. Instead, any values that you need to pass to $form_state['values'] can be declared in the $form array as #type => value.
3. sign_for_acknowledgement_form_validate(): empty function, so remove it.
4. sign_for_acknowledgement_install(): filter_xss() is not necessary here since no user provided text is involved. Please read http://drupal.org/node/28984 again. Looks like you changed the code because of a false positive in coder review.
5. _sign_for_acknowledgement_create_display_table(): I think you can just assume that the $rows_limit is numeric, better create a form validation handler for you admin form where check that the variable is an interger.
6. admin/config/people/acknowledge: I think the fields for first and last name should be selectboxes of existing user fields.
7. _sign_for_acknowledgement_create_display_table(): this is vulnerable to XSS exploits. If a user enters <script>alert('XSS');</script> as first name then there is a nasty javscript popup when the table is displayed. You need to sanitize all user provided input before printing. Please read http://drupal.org/node/28984 again. Adding security tag, please don't remove it, we keep that for statistics and to show examples of security problems.

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

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/sign_for_acknowledgement.module
  --------------------------------------------------------------------------------
  FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
  --------------------------------------------------------------------------------
   295 | WARNING | Class name must be prefixed with the project name
       |         | "sign_for_acknowledgement" (omitting underscores)
  --------------------------------------------------------------------------------
  

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.

But otherwise looks RTBC to me now. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Assigning to zzolo as he might have time to take a final look at this.

no objections for more than a week, so ...

Thanks for your contribution, pagolo!

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:

• 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 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.