[D7] Cleverreach

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

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.

1. It appears you are working in the "master" branch in git. You should really be working in a version specific branch.

2. cleverreach.module

line 175
Functions and variables should be named using lowercase, and words should be separated with an underscore. Rename _cleverreach_get_groupName to _cleverreach_get_group_name

line 231
Functions and variables should be named using lowercase, and words should be separated with an underscore. Rename _cleverreach_build_blockForm to _cleverreach_build_block_form

3. cleverreach.admin.inc
line 159
I think it would be correct to move this hook to cleverreach.module file.

Sorry for my english.

You should just have the 7.x-1.x branch. I understand why you put dev in it, but that's not exactly the pattern.

On the Overview page, when you click 'Add Block" you lose context with the tabs (no longer active).
- http://localhost:8888/admin/config/services/cleverreach/addblock

All userfacing text should run through t() for translation
Cleverreach admin overview
line 29 in cleverreach.module:
'description' => 'Cleverreach admin overview.',
and many others in cleverreach_menu()

~R~

Hi kasalla,

Here are the few additional comments on your module code.

.module
-------
- _cleverreach_build_block_form() have got html in the code which is not recommended as per standards though its already there in few of existing modules as well however you should think about removing that.

- All control and conditional blocks like, foreach, if etc should have a blank line before and after the block.

- _cleverreach_group_assign_validate() is using a regex to validate the email address. Please use valid_email_address() which is a drupal method to validate it.

- Line#315 have a static query which is using db_select, please use db_query to make the process faster if there is no dynamic database query.

.cleverreach.admin.inc
----------------------
- Line#12 Use db_query as this is also a static query.

- Also this file is also a candidate for few of the changes which were suggested in the above file.

.install
-----------------------
- Need not to provide empty indexes/unique keys/foreign keys.

I think that should be it once you made changes to your code.

Hi kasalla,

if I have a field of type gender imported from cleverreach, the form field in the subscription box should be rendered as select box instead of a text field. The values of the type gender may contain male, female, mr, mrs, herr,frau, mann, frau, ... as described in the Cleverreach API documentation.

Kind regards
Silbermann

Great work! I will use this module in future projects.

Little thing in line 447 in file cleverreach.admin.inc. Sometimes the textarea to determine the select box options disappears if i changed the cleverreach group on the block edit page. To prevent this change the line 447 in:

$display = isset($form_state['input']['cr_grp_wrapper']['table'][$value["key"]]) ? $form_state['input']['cr_grp_wrapper']['table'][$value["key"]]['display'] : $display_options;

On my wishlist is an optional opt-in e-mail. Should be possible with the formsSendActivationMail function of the cleverrach api. Only a suggestion, but not necessary.

Kind regards

Changing status to needs work

Looks good. No more errors found.

Last remark: to help other developers to understand your code quickly you could provide some inline comments. More details about module documentation here.

But I am not sure if it is really necessary in this module. Most of it should be self-explaining. So I set this project application to reviewed & tested by the community.

Kind regards

• In cleverreach_install() don't set variable defaults there. The right way to do it is provide a default when reading the variable with variable_get('varname', 'defaultvalue')
• If you're using the SoapClient class, implement hook_requirements() and check for it in your .install file
• You have some style issues reported here: http://pareview.sh/pareview/httpgitdrupalorgsandboxkasalla2017855git. It also includes some security warning about using $form_state['input']

Otherwise this looks pretty good to me.

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

Thanks for those updates!

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

The following git branches do not match the release branch pattern, you should remove/rename them. See http://drupal.org/node/1015226

  remotes/origin/7.x-1.x-dev

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/cleverreach.admin.inc
  --------------------------------------------------------------------------------
  FOUND 0 ERROR(S) AND 2 WARNING(S) AFFECTING 2 LINE(S)
  --------------------------------------------------------------------------------
   84 | WARNING | Do not call theme functions directly, use theme('fieldset',
      |         | ...) instead
   85 | WARNING | Do not call theme functions directly, use theme('fieldset',
      |         | ...) instead
  --------------------------------------------------------------------------------
  

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. cleverreach_build_block_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'. Same in _cleverreach_del_block_form().
2. _cleverreach_group_assign(): instead of using time() multiple times you could just use REQUEST_TIME.
3. _cleverreach_admin_overview(): looks vulnerable to XSS exploits, the group name is printed unsanitized to the table. I could not exploit this since I could not create a malicious group name on Cleverreach, but you should sanitize the user provided text here anyway. Make sure to read https://drupal.org/node/28984 again.
4. _cleverreach_admin_overview(): Do not call theme functions here, just return a render array and Drupal will render it later for you.
5. _cleverreach_add_block_form(): doc block is wrong, this is not a hook. See https://drupal.org/node/1354#forms . Same for _cleverreach_del_block_form_submit(), please check all your functions.
6. theme_addblock_theming(): all theme function that you define need your module name in it to avoid name clashes.
7. _cleverreach_del_block_form_submit(): do not use drupal_goto() here, use $form_state['redirect'] instead.

I'm a bit on the fence whether the potential XSS issue is a blocker, but since I could not exploit it currently ...

Thanks for your contribution, kasalla!

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.