Last updated by naveenvalecha on Wed, 2015-03-04 19:00
This wiki page is created to keep track of naveenvalecha's manual reviews of projects in the project application queue. As a reviewer I hope to get more advice from more experienced reviewers. Please use the comment field to add your advice, corrections, and observations.
I also lead a sprint for 7 peoples at DrupalCamp Mumbai 2015 about this subject and was able to to provide mentorship about the process of applying for a full project and how to review the project applications.
Some projects have multiple reviews; first is linked.
Found security issues in contrib modules that I addressed to @klausi and filed issues on the security.drupal.org
Reviews List:
Full Project Promote Issues:
- [D7] ShareThisField
- [D7] Numbered Multivalue Fields
- [D7] webform_country_list
- [D7]Reach us
- [D7]Bulk delete 301
- [D7] IXI: XML Importer
- [D7] Select Role
- [D7] Video Embed Instagram
- [D7]Page Background
- [D7] vCita
- [D7] SearchIndex Wipe
- [D7] Drawingfield
- [D7] Node Reference Subqueue
- [D7] Path Alias Picker
- [D7] IP2Location Module
- [D7] jQuery Carousel
- [D7] Message Private
- [D7] Social Share Buttons
- [D7] Dynamic Zendesk Forms
- [D7] Pushup Social
- [D7] Anonymous Suggestion Box
- [D7] Drop Down Login
- [D7] Listserv
- [D7] Listserv
- [D7] Lagotto services
- [D7] cmood
- [D7] Read more ajax
- [D7] Heap Analytics
- [D7] User Recording
- [D7] User Recording
- [D7] VoiceCommander
- [D7] Restricted Links
- [D7] Site Info
- [D7] usercancel_contentassigntoadmin
- [D7] Mini Blocks
- [D7] Page Guide
- [D7] Homebox Packery
- [D7] shs search api
- [D7] Ubercart Wallet one
- [D7] Node title help text
- [D7] Ubercart Element Payment Gateway
- [D7] Menu block ignored links
Single Project Promote Issues:
Security Issues:
Comments
Thank you for your reviews!
Thank you for your reviews! The security issue mentioned does not seem to be a XSS issue because the value is then used in an arithmetic operation ("+" and "-"), so PHP will just cast it to a number. Make sure to actually try to exploit XSS issues by inserting/sending script tags and then observe any alert() popups for example.
When you finish a review you should always come to a conclusion: are there blocking issues that have to be resolved (such as security issues)? Then the application should be set to "needs work". Are there no serious issues left in the code? Then the application should be set to "reviewed and tested by the community". Could you go back to all your reviews, check again and flip the status into one direction or the other? In some reviews there were only very minor issues such as usage of hook_form_FORM_ID_alter() vs hook_form_alter(), which does not justify "needs work". See also my comment on not being too strict: https://groups.drupal.org/node/434943#comment-1051778
Quoting one of your reviews: "ixi.install : Set these values of the variables a default value instead of setting on the hook_install." This is quite cryptic. What do you mean here? You should always be specific what people should actually change, so if I read this correctly: "Do not set variables in hook_install() with variable_set(), you can just use default values with any variable_get() call".
Thanks for a nice feedback!
Thanks klausi for nice feedback!
Thanks for letting me know about the XSS issue.I have updated my reviews listing.I am view the reviews done by you and other administer and also thorougly view the security reviews and learning the useful things.
I'll keep take care of all your points mentioned by you and I'll use your feedback & suggestions in my next reviews.
Thanks klausi I'll take care in my future reviews and will do the reviews with Full front nicety.
I'll get back to all my reviews and update them with your valuable suggestion.
Thanks.I have updated the review https://www.drupal.org/node/2337931#comment-9189693
Again Thanks for mentoring my code reviews.your mentoring will help me for good code reviews in future.
Naveen Valecha
https://www.valechatech.net
Updated my all reviews
@klausi,
I have updated my all reviews and updated them with suggestions.
I am looking at the security reviews done by git admins to learn more and reviewed the project applications in case of security.
I have a question regarding the project applications which has code less than 120 lines as you have specified to @pushpinder here that we manually approve the short projects.So I have made this application RTBC as I did not find the release blocker so far. https://www.drupal.org/node/2338629
Naveen Valecha
https://www.valechatech.net
Yes, that is fine. We use the
Yes, that is fine. We use the "PAReview: single project promote" tag to mark such short applications.
Adding this here, instead of
Adding this here, instead of on the issue.
The main application blockers are security problems, licensing issues, and third-party code. The fourth are "major API problems". To quote @klausi from my mentoring:
So, a few missing t() here and there are fine. Total disregard for translation isn't. A few bad indents, OK? Not following any Drupal coding standard, blocker. Not doing something quite the Drupal way, probably OK? Bypassing the API in a big way (like accessing $POST instead of $form_state['values']), blocker.
I would suggest looking at the feedback on the mentoring for the last few admins (me, heddn, and er.pushpinderrana) for more advice from klausi. Also go through the recently approved applications, and look for reviews from the admins to see what they did block on, and what they didn't.
Thanks @mpdonadio
Thanks @mpdonadio
I will consider these in my future reviews.I will continue my learning, hope day by day you will see improvements in the reviews.
Naveen Valecha
https://www.valechatech.net
Thanks @naveenvalecha
As your guidance regarding promote to full project, review project, creating own wikipage in drupal. I can able to promoted my own project from sandbox. Also I can able to review other projects.Thanks @naveenvalecha for your kind help. I appreciate your efforts.
Thanks for words
Thanks for your words!
I would love to see that you will continue reviewing the project applications.
Naveen Valecha
https://www.valechatech.net
Naveen has just been made a
Naveen has just been made a git admin at https://www.drupal.org/node/2445643
I think he has a good understanding of the project application process and has also the necessary skills to identify problems with applications (like security issues).
Therefore I'd like to welcome you to the review admin team and feel free to use your new powers to review and approve RTBC applications! (instructions at https://www.drupal.org/node/1125818 )
Thanks!
Thanks Klausi,
I will do my very best to surpass your expectations and will continue my learning and hope day by day you will see improvements. Thanks again.
Naveen Valecha
https://www.valechatech.net