Reviews and Mentoring for naveenvalecha

You are viewing a wiki page. You are welcome to join the group and then edit it. Be bold!

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:

  1. [D7] ShareThisField
  2. [D7] Numbered Multivalue Fields
  3. [D7] webform_country_list
  4. [D7]Reach us
  5. [D7]Bulk delete 301
  6. [D7] IXI: XML Importer
  7. [D7] Select Role
  8. [D7] Video Embed Instagram
  9. [D7]Page Background
  10. [D7] vCita
  11. [D7] SearchIndex Wipe
  12. [D7] Drawingfield
  13. [D7] Node Reference Subqueue
  14. [D7] Path Alias Picker
  15. [D7] IP2Location Module
  16. [D7] jQuery Carousel
  17. [D7] Message Private
  18. [D7] Social Share Buttons
  19. [D7] Dynamic Zendesk Forms
  20. [D7] Pushup Social
  21. [D7] Anonymous Suggestion Box
  22. [D7] Drop Down Login
  23. [D7] Listserv
  24. [D7] Listserv
  25. [D7] Lagotto services
  26. [D7] cmood
  27. [D7] Read more ajax
  28. [D7] Heap Analytics
  29. [D7] User Recording
  30. [D7] User Recording
  31. [D7] VoiceCommander
  32. [D7] Restricted Links
  33. [D7] Site Info
  34. [D7] usercancel_contentassigntoadmin
  35. [D7] Mini Blocks
  36. [D7] Page Guide
  37. [D7] Homebox Packery
  38. [D7] shs search api
  39. [D7] Ubercart Wallet one
  40. [D7] Node title help text
  41. [D7] Ubercart Element Payment Gateway
  42. [D7] Menu block ignored links

Single Project Promote Issues:

  1. [D7] Static Server

Security Issues:

  1. [D7] Social Share statistics
  2. [D7] cmood : #1, #2, #3
  3. [D7] VoiceCommander
  4. [D7] Static Content Manager
  5. [D7] Panel Parallaxe
  6. [D7] Reactor Integration
  7. [D7] Permanent Filelink
  8. [D7] Sweetcaptcha
  9. [D7] Selfi

Comments

Thank you for your reviews!

klausi's picture

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!

naveenvalecha's picture

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.

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

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.

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

Updated my all reviews

naveenvalecha's picture

@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

Yes, that is fine. We use the

klausi's picture

Yes, that is fine. We use the "PAReview: single project promote" tag to mark such short applications.

Adding this here, instead of

mpdonadio's picture

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:

Project application reviews are basically sanity checks, so while providing all details what can be improved is very valuable we have to consider what issues are really blocking approval. If we are confident that the people basically know what they are doing then we should not hold them back. With the exception of security issues and licensing issues.

I personally set applications to needs work if the API usage is so painfully wrong or the spaghetti code issues just keep piling up. Fortunately people fix all the things from pareview.sh, so at least we don't have to complain about coding standards anymore.

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

naveenvalecha's picture

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.

Thanks @naveenvalecha

Rahul Seth's picture

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

naveenvalecha's picture

Thanks for your words!
I would love to see that you will continue reviewing the project applications.

Naveen has just been made a

klausi's picture

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!

naveenvalecha's picture

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.

Code Review for opt in security advisory coverage | Home

Group organizers

Group notifications

This group offers an RSS feed. Or subscribe to these personalized, sitewide feeds:

Hot content this week