Name : Smart Tribune AddSnippet

Description :
A module that allows admins to easily insert Smart Tribune snippet and control its visibility similar to the core Drupal block module.
It is used to inject Smart Tribune code snippet just before end tag which is the recommended placement for Smart Tribune snippet.

Smart Tribune is a brand new SaaS software that let you personalize your online customer relationship. This module will help Website admin to integrate as easily as possible our snippet into their website.

Sandbox url : http://drupal.org/sandbox/slastmann/1846666

Git link : The repository can be found here: git clone http://git.drupal.org/sandbox/slastmann/1846666.git smart_tribune_addsnippet

Intended Drupal core version : Drupal 7.x

Other manual project application reviews:
http://drupal.org/node/1910086#comment-7038026
http://drupal.org/node/1935836#comment-7152982
http://drupal.org/node/1935118#comment-7152824

CommentFileSizeAuthor
#24 coder-results.txt816 bytesklausi
#10 Selection_013.png45.45 KBvladimir-m
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

samail’s picture

Issue summary: View changes

Add missing details

gazoakley’s picture

Status: Needs work » Needs review

Hi samail,

Thanks for submitting a project application for review. To help you track the status of your application you might want to enable email notifications for changes to your application:

http://drupal.org/project/issues/subscribe-mail/projectapplications

You might also want to participate in the review bonus program - this should help ensure your application is review quickly:

http://drupal.org/node/1410826

The PAReview tool identified a few issues which need addressing:

http://ventral.org/pareview/httpgitdrupalorgsandboxslastmann1846666git

  • Please remove the LICENSE.txt file. Drupal.org will add the appropriate version automatically during packaging so your repository should not include it.
     
  • Your .info file does not match the name of your module - please rename "smart_tribune_addsnippet.info" to "smarttribune_snippet.info" or the PAReview tool will continue to show function prefix errors
     
  • The Drupal Code Sniffer also found errors - please see the above link
     

Please check/fix these issues before changing the status back to "needs review"

gazoakley’s picture

Status: Needs review » Needs work
Artusamak’s picture

Status: Needs review » Needs work

First a word of code review after passing it through PAReview:

  • Remove LICENSE.txt, it will be added by drupal.org packaging automatically.
  • Remove the translations folder, translations are done on http://localize.drupal.org

Then, beware of the comments length, they are longer than 80 chars, you should also have sentences everywhere, it means that your comments should start with a capital letter and end with a period.

In the hook_schema():

  • I think that the export section of this hook is a copy paste that should be removed.
  • There is a typo: 'The primary indentifier of Smart Tribune snippet'
  • tcid = ? Maybe that a more explicit name could be helpful (not critical at all)
  • Column region 'length' => 20, 20 can be a low limit for certain users, keep it at 255 chars.
  • Use the serialize flag (http://api.drupal.org/api/drupal/includes!database!schema.inc/group/sche...) to leverage the auto serialization / deserialization

In theme_smarttribune_snippet_overview_table():

  • $regions = unserialize($form['regions']['#value']);
    Remove if you are using the serialize flag.
  • if (empty($regions['page_bottom']))
    Use a renderable array if possible, something like return array('#markup' => $output); and wrap it in a

    (#markup or #html_tag).

  • Around foreach ($snippets as $id => $snippet)
    $actions = array();
    Useless :)
    'fragment' => '',
    Useless :)
  • The enable / disable link should probably converted to a renderable array to use an ajax command rather than a callback using exit() (http://api.drupal.org/api/drupal/includes!ajax.inc/function/ajax_command...)
  • The usort() call should not be there if you trust the weight attribute, have a look to the code of core handling blocks listing to see what's the good practice there.

In smarttribune_snippet_edit_form() / smarttribune_snippet_delete_form()

  • Handle the 404 in an access callback

I didn't dive into the queries, JS and rest of the module file yet.

Artusamak’s picture

Status: Needs work » Needs review

Alright, some love has been given to the module, quite a lot of things have been cleaned and the module now matches Drupal expectations, PAReview is also happy http://ventral.org/pareview/httpgitdrupalorgsandboxslastmann1846666git-7....

This project looks ready to me to be promoted, as a piece of advice for the future, you may want to consider using a dedicated entity type to store your snippets but it's not mandatory.

alexmoreno’s picture

Nothing to add, the code passes the tests, it is very well documented, easily readable... it even includes a readme with install instructions.

The only last thing is to collaborate in another (at least) three reviews, so your project application can proceed to the next phase.

Thank you.

samail’s picture

Thanks for having reviewed my module.

I ll start to help in module review too.

samail’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +PAreview: review bonus

Hi,

Following what has been said in the last comment ( http://drupal.org/node/1846814#comment-7005246 ), I added PAReview bonus tag as have now added 3 other project reviews to the issue summary.

Waiting for project application validation now
Thx in advance

klausi’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: -PAreview: review bonus

Removing review bonus tag, no reviews listed in the issue summary.

Also don't RTBC your own issues, see http://drupal.org/node/532400

klausi’s picture

Issue summary: View changes

extra infos

samail’s picture

Hi Klausi,

My bad i really thought i added my reviews in the summary but probably did not populate after preview !

I did not want RTBC because i was sure other project review links were added into my project summary so sorry about about this one.

There are now well pasted in there so waiting for your validation.

Thx in advance

vladimir-m’s picture

Status: Needs review » Needs work
FileSize
45.45 KB

Hello samail,

Thank you for great module.

1. Please add git link, for example: "The repository can be found here: git clone http://git.drupal.org/sandbox/slastmann/1846666.git smart_tribune_addsnippet".
2. In "/admin/structure/smarttribune_snippet" then I click to "Add Smart Tribune Snippet" I get this warning (take a look to attached screenshot).
3. Also then I insert in to "Code" field, "<" string I get a lot of warnings. Can you put a validation to this field?

vladimir-m’s picture

Issue summary: View changes

Addind other project review into project summary

samail’s picture

Status: Needs work » Needs review

Hi,

Thx vladimir-m for the review.

Everything is now fixed (except your raised "<" string issue that i can not reproduce at all)

Waiting for validation.

Thx in advance

klausi’s picture

Don't forget to add the "PAReview: review bonus" tag as indicated in #1410826: [META] Review bonus, otherwise you won't show up on my high priority list.

samail’s picture

Issue tags: +PAreview: review bonus

Add "PAReview: review bonus" tag as requested

klausi’s picture

Issue tags: -PAreview: review bonus

Removing review bonus tag, you have not done any manual review, you just posted the output of an automated review tool. Make sure to read through the source code of the other projects.

klausi’s picture

Issue summary: View changes

Add git link to summary

klausi’s picture

Issue summary: View changes

removed automated reviews

samail’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +PAreview: review bonus

Manual review of other projects : done
Adding "PAReview: review bonus" tag

samail’s picture

Status: Reviewed & tested by the community » Needs review
lorenlang’s picture

Status: Needs review » Needs work

Not much to add re: the code quality. There are a few typos and grammatical errors that could be cleaned up to make the end user experience just a bit better.

  • README.txt: Line 20 - 'recommand' should be 'recommend'
  • README.txt: Line 40 - 'visiblity' should be 'visibility'
  • smarttribune_snippet.admin.inc: Line 151 - 'Paste your Smart Tribune snippet here, you can fetch...' should be 'Paste your Smart Tribune snippet here. You can fetch...'
  • smarttribune_snippet.admin.inc: Line 188 - 'Smart Tribune snippet are disabled by default, so you...' should be 'Smart Tribune snippets are disabled by default so you...'
  • smarttribune_snippet.module: Line 40 - 'Create Smart Tribune snippet' should be 'Create Smart Tribune snippets'

Nitpicky, I know, but every little bit helps, right?

samail’s picture

Status: Needs work » Needs review

Hi llang,

Thx for the feedback. The few grammatical errors are fixed.

jared_sprague’s picture

Hello!
Here is my manual review:

Project Page

Your project page needs the most work, compared to everything else. Currently it looks like the project page is just pasted from the project application issue.
- The Project Page should follow this template: http://drupal.org/node/997024, you need sections like Overview, Features, Documentation etc.
- Remove the links to project application reviews
- remove git repo link
- Since this module is an extension of Smart Tribune, There should be information directing me to learn more about what Smart Tribune is. After reading your project page, README.txt, source code, and testing your app, I still have very little idea what Smart Tribune is. You need to include more information about Smart Tribune, or link documentation to your user to learn more about Smart Tribune.

README.txt

- missing 'is' between 'which' and 'the' on line 7
- You may want to include the same relevant sections in your README.txt file that are included in your project page listed in Tips for a great project page, obviously you wouldn't put stuff like screenshots in your README, but sections like Install, Features, Links to Documentation should definitely be in your README and be consistent with your Project Page.

smarttribune_snippet.module

- You're missing t() functions on title and description elements in this function: smarttribune_snippet_menu

Security

- Your module accepts any javascript, I entered javascript that displayed popups. I'm assuming that you Smart Tribune generates javascript to be pasted into the code field? If so I would consider sanitizing the input of this field to only allow Smart Tribune generated javascript and strips out everything else.

Misc.

- The top level git directory dose not match the module name, it would look cleaner if you could make these consistant. Either change the directory name to "from smart_tribune_addsnippet to-> smarttribune_snippet" to match your module name, or change the module name to "smart_tribune_addsnippet" either way it should be consistent.
- All of your top level files are executable, that is not necessary, I recommend setting these to 644

Testing

- Your module works fine without errors, using just some random javascript.
- Consider providing some instructions for testing Smart Tribune snippets.

Your code looks clean and well written! Now you just need to clean up the random stuff outside of the code, and this module will be great!

Artusamak’s picture

Status: Needs review » Needs work

Comment on the previous review, the t() call for menu title is to avoid because titles are wrapped in the title callback property of the hook_menu() which is by default equals to t().

samail’s picture

Status: Needs work » Needs review

All done except JS sanitizing. Will think about it and implement as soon as possible.

jared_sprague’s picture

Your project page and README.txt look much better!
Here are a few minor things I noticed on your project page:
- ecven is misspelled.
- use periods consistently in your bullet lists.
- Remove the link to your sandbox project at the bottom, it just points back to the same page.
- If possible change the directory name of your git repo from "smart_tribune_addsnippet" to "smarttribune_snippet" so it matches the name of your module.

Other than that it looks good to me!

samail’s picture

Hi,

All done except directory name change. We will carefully choose final URL when the application will be promoted.
Thx again.

klausi’s picture

Assigned: Unassigned » sreynen
Status: Needs review » Reviewed & tested by the community
Issue tags: -PAreview: review bonus
FileSize
816 bytes

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

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. So essentially you are cloning the block system. Why can't you expose your snippets as blocks that hold the information? You would not have to display the blocks, but you could do the same page alter logic as you do now.
  2. smarttribune_snippet_admin_overview(): that page callback is not needed if you just add the CSS and JS with #attached in the form constructor. And in smarttribune_snippet_menu() you you use drupal_get_form as page callback and point directly to the form building function.
  3. smarttribune_snippet_ajax_disable(): use drupal_json_output() here. And use drupal_exit() instead of exit; if you have to.
  4. smarttribune_snippet_load(): Do not use db_select() for simple static queries, use db_query() instead. See http://drupal.org/node/310075

Overall no blockers, so I guess this is RTBC. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

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

sreynen’s picture

Assigned: sreynen » Unassigned
Status: Reviewed & tested by the community » Fixed

Thanks for your contribution, samail!

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:

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 reviewers as well.

samail’s picture

Hi everyone,

@sreynen : Thx a lot for my account update. Be sure that i ll continue to take part of the review process as much as i can. :-)

@klausi: thx a lot for your feedback.
I did the 3. and 4. changes and i ll think about the 2 others later.

Kind regards
samail

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary: View changes

Add manual review links