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
Comment | File | Size | Author |
---|---|---|---|
#24 | coder-results.txt | 816 bytes | klausi |
#10 | Selection_013.png | 45.45 KB | vladimir-m |
Comments
Comment #0.0
samail CreditAttribution: samail commentedAdd missing details
Comment #1
gazoakley CreditAttribution: gazoakley commentedHi 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 check/fix these issues before changing the status back to "needs review"
Comment #2
gazoakley CreditAttribution: gazoakley commentedComment #3
ArtusamakFirst a word of code review after passing it through PAReview:
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():
'length' => 20,
20 can be a low limit for certain users, keep it at 255 chars.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).
foreach ($snippets as $id => $snippet)
$actions = array();
Useless :)
'fragment' => '',
Useless :)
In smarttribune_snippet_edit_form() / smarttribune_snippet_delete_form()
I didn't dive into the queries, JS and rest of the module file yet.
Comment #4
ArtusamakAlright, 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.
Comment #5
alexmoreno CreditAttribution: alexmoreno commentedNothing 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.
Comment #6
samail CreditAttribution: samail commentedThanks for having reviewed my module.
I ll start to help in module review too.
Comment #7
samail CreditAttribution: samail commentedHi,
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
Comment #8
klausiRemoving review bonus tag, no reviews listed in the issue summary.
Also don't RTBC your own issues, see http://drupal.org/node/532400
Comment #8.0
klausiextra infos
Comment #9
samail CreditAttribution: samail commentedHi 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
Comment #10
vladimir-m CreditAttribution: vladimir-m commentedHello 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?
Comment #10.0
vladimir-m CreditAttribution: vladimir-m commentedAddind other project review into project summary
Comment #11
samail CreditAttribution: samail commentedHi,
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
Comment #12
klausiDon'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.
Comment #13
samail CreditAttribution: samail commentedAdd "PAReview: review bonus" tag as requested
Comment #14
klausiRemoving 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.
Comment #14.0
klausiAdd git link to summary
Comment #14.1
klausiremoved automated reviews
Comment #15
samail CreditAttribution: samail commentedManual review of other projects : done
Adding "PAReview: review bonus" tag
Comment #16
samail CreditAttribution: samail commentedComment #17
lorenlang CreditAttribution: lorenlang commentedNot 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.
Nitpicky, I know, but every little bit helps, right?
Comment #18
samail CreditAttribution: samail commentedHi llang,
Thx for the feedback. The few grammatical errors are fixed.
Comment #19
jared_sprague CreditAttribution: jared_sprague commentedHello!
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!
Comment #20
ArtusamakComment 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().
Comment #21
samail CreditAttribution: samail commentedAll done except JS sanitizing. Will think about it and implement as soon as possible.
Comment #22
jared_sprague CreditAttribution: jared_sprague commentedYour 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!
Comment #23
samail CreditAttribution: samail commentedHi,
All done except directory name change. We will carefully choose final URL when the application will be promoted.
Thx again.
Comment #24
klausiReview 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:
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.
Comment #25
sreynen CreditAttribution: sreynen commentedThanks 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.
Comment #26
samail CreditAttribution: samail commentedHi 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
Comment #27.0
(not verified) CreditAttribution: commentedAdd manual review links