Reviews and mentoring of user gisle

Events happening in the community are now at Drupal community events on www.drupal.org.
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 gisle's manual reviews of projects in the project application queue. As a rookie reviewer I hope to get some advice from more experienced reviewers. Please use the comment field to add your advice, corrections, and observations.

Partcipation in Project Applications project.

Reviews

The status is just to help myself keeping track of these, and may not always be correct. Starred items(*) indicate that a security issue was identified in the review.

  1. [D7] twentyeleven (theme) [F]*
  2. [D7] wheke (profile) [F]
  3. [D7] Upcoming Content [F]
  4. [D7] Ajax Regions [F]
  5. [D7] Field Concat Display [F]
  6. [D7] Field Context [F]
  7. [D7] Sticky Sharrre Bar [F]
  8. [D7] LimitVisit [F]*
  9. [D7] User Revision Edit [F]
  10. [D7] Login tracker [F]
  11. [D7] Codit [F]
  12. [D7] Resizable [F]
  13. [D7] Events Conferences Theme (theme) [C(WF)]*
  14. [D7] MobEye [C(WF)]
  15. [D6] ImageCache Use Original [C(WF)]
  16. [D7] Account Author [RTBC]
  17. [D7] Percentage Scale [C(WF)]
  18. [D7] Image progressive [C(WF)]
  19. [D7] UserBallot integration [C(WF)]
  20. [D7] Merlin [C(WF)]
  21. [D7] Simplenews Status Reset [C(D)]
  22. [D7] Google Plus Login [C(WF)]
  23. [D7] IP2Location Module [C(WF)]
  24. [D7] uc_fancy [C(WF)]
  25. [D7] SetCron [C(WF)]
  26. [D7] Maths Captcha [C(WF)]
  27. [D7] Insha (theme) [F]
  28. [D7] App Link [F]
  29. [D7] Commerce EST Direct [C(WF)]
  30. [D7] Aramex API Integration [C(D)]
  31. [D7] Numbered Multivalue Fields [F]
  32. [D7] Guardian news [NW]*

Comments

Welcome! I saw on your

klausi's picture

Welcome!

I saw on your account that you are not a git vetted user yet. Before we start mentoring you to become a code review administrator you should go through the application process yourself. Do you have any project at hand that you would like to promote?

Flexi Access

gisle's picture

I am using an abandoned module called Flexi Access on my site. My project is to turn this module into a user friendly and flexible client for the ACL module. The goal is to use this module to provide the site the administrator with a form interface that let him/her create and maintain named ACLs for the ACL module (the ACL module have all the functionality I need, but it comes without a user interface on its own).

Here is a pointer to the project: https://drupal.org/project/flexiaccess

Thanks for a great

jthorson's picture

Thanks for a great demonstration of initiative, gisle!

The approach you discuss (taking over an abandoned project) is one way to bypass the project applications review process ... in other words, once your webmaster application is approved, you will have full rights to the abandoned project's repository, including push and editing as you described. However, that doesn't necessarily get you the 'git vetted user' role that klausi mentioned ... a bit of a flaw in our processes.

If your plans in taking over of the Flexi Access module include major refactoring of the code, then we might be able to use that project as the target of reviews for the 'git vetted user' status ... but I suspect that many reviewers will balk at reviewing a clone of an existing project as it stands today, since they prefer to see your own code (and I'm assuming the code in the sandbox still closely resembles the original project). If my assumption is wrong, and your sandbox does include a ground-up rewrite of the original project; then please disregard this aspect of my comment. :)

I thought that the review

frob's picture

I thought that the review process included leeway for review of projects that have been taken over - So long as a good representation of your own code is available for review. Also, this might be a little more difficult as most existing project don't pass the pareview script.

I'd agree with your 'So long

jthorson's picture

I'd agree with your 'So long as a good representation of your own code is available' caveat. This does, however, leave some room for interpretation ... which may not be consistent from one reviewer to the next.

I'm not trying to discourage the approach ... just flagging that the sandbox should contain the refactoring before submitting the project to the application queue.

Thank you for your comments

gisle's picture

Thank you for your comments, jthorson and frob.

My main motivation for doing these reviews is not to gain the 'git vetted user' role. At the moment, I just want to get more involved with the community and learn how to make an acceptable Drupal project.

However, getting such a role will be a nice bonus, since the process for being granted maintainership of abandoned projects seems to be quite backlogged (there are some quite old applications in the webmasters queue flagged "needs review" without a single review or comment).

As for the state of the code in my project, it is true that it closely resembles the original project (a diff will show those interested what I've changed), since my contribution so far is only bug fixes. However, it passes the PAReview script and coder (and the original project didn't).

However, I haven't submitted this project as target for reviews for 'git vetted user role' promotion, and have no plans to do so in the near future. Currently, I'm only here to learn.

Git vetted status before git admin

cubeinspire's picture

The reason to ask you the git vetted status before making you git admin is, in my opinion, that passing by the process will allow you to have a practical experience on how the project application review works, and how other reviewers analyse the code you want to contribute.

Personally I can tell you that it's a very good experience and the remarks from other developers helped me to have a deeper understanding of drupal code standards and common security issues (that are quite frequently forgotten by new contributors when coding in a rush)

cube inspire - web design and web development solutions !

logicdesign, thank you for

gisle's picture

logicdesign,
thank you for your comments. I certainly see your point.

Unfortunately, at the moment, I don't have a good new project to submit into the "git vetted" process. I don't want to "make up" a project for this. If I am going to do it, it should be something I would really need and use for my sites (otherwise, I may not be motivated enough to maintain it properly).

I think the "submit a new project for review to become git vetted"-process set up by klausi is a great idea, but it has the unfortunate side-effect that it only motivates people interested in becoming "git vetted" to create new projects. With 19086 full projects already on http://drupal.org/project, I am not sure that what the Drupal community really needs is more new projects.

In my experience, there are too many poorly maintained (or not maintained at all) projects in the Drupal projects namespace. Most of those can of course be ignored. But some of those I use, and I really would see the maintenance status of those improve.

At the moment, I use most of the time I have available for Drupal work to try to "fix" the abandoned modules my sites depend on. But, as pointed out by jthorson, there is an alternative process for dealing with unsupported (abandoned) projects, outlined here: http://drupal.org/node/251466

However, I wonder if this process is working? Because these projects are abandoned, they're buggy and and they attract few users. As a result, almost nobody is interested in reviewing the patches submitted for them by the people who want get them maintained and functional again. Suggested patches remains in the "needs review" state without a single reply for very many weeks. Example: http://drupal.org/node/1405972

I think that someone who

greggles's picture

I think that someone who excels at reviewing will quickly show themself to be worth an exception to this rule :) Which is to say: just keep on doing great reviews.

I believe there's also a way for someone who has substantially rewritten an abandoned module to get their review based on the rewritten module, so that could work in your case.

Apprentice project

gisle's picture

I now have a project that I want reviewed in order to become git vetted user. The name of the project is Anonymous publishing.

The application is in the issue queue for Drupal.org Project applications.

I understand that it is now possible to apply for git vetted access according to the procedures listed under the heading Abandoned Module Applications.

Final review of

mpdonadio's picture

Final review of https://www.drupal.org/node/2287023 was just completed, and you now have the vetted user permission. You have been fairly active with reviewing. I assume you still want to purse becoming a review administrator?

Yes, I plan to pursue becoming a git admin

gisle's picture

If you or any other git admin have anything to say about by review "style" (in particular about things that can be improved), I would appreciate that.

The reviews I looked at are

greggles's picture

The reviews I looked at are extremely detailed, that seems great. You seem to use a lot of templates and links to resources on drupal.org, also seems great. You have a very strong attention to detail, that is also great! You found some security issues, which is a subject near and dear to my heart.

Your persistence on the licensing issue is probably pretty frustrating to applicants. They likely see other people ignoring it and feel like they are being singled out. However, you are just following the stated policy, so...it's hard to say that you're doing it wrong.

I hope you will keep up the good work!

+1 to what greggles said:

klausi's picture

+1 to what greggles said: great attention to details, good understanding of our workflows and I think we settled the licensing discussion on derivative work. You took initiative to start discussions about things that are not clear yet or things that could be improved, I really like that!

I was a bit surprised to find XSS security issues in your own project application code, but you fixed them in no time and then also found security issues in other projects. BTW: I marked your release at https://www.drupal.org/node/2293427 as security update ;-)

I was also a bit frustrated about one issue where you said something like "many contrib modules have security issues". I find that to be an exaggeration. If you find one ==> please report it to the security team. We have a pretty good process to deal with security issues on drupal.org, so there should actually be very few contrib modules that linger around with obvious security issues.

Otherwise you helped a lot and I think you are an awesome contributor. Let's wait a couple more weeks and keep collecting your reviews here, then I think we can make you an admin to also approve applications :-)

Thanks for the feedback!

gisle's picture

klausi wrote:

I was a bit surprised to find XSS security issues in your own project application code, but you fixed them in no time and then also found security issues in other projects.

Yeah, I am really embarrassed about that. While I have read greggles book about security, I realize that I've not fully understood XSS security issues before you pointed them out in my own code. As I said: It has been an educational experience.

I was also a bit frustrated about one issue where you said something like "many contrib modules have security issues". I find that to be an exaggeration.

It was in a response to an applicant who insisted his (IMHO hard to configure) project did not need to be properly documented because there exist many other projects on Drupal.org with poor documentation. I responded with something like "Many contrib modules also have more bugs than a rain forest, and more security holes than a colander." It was intended as humor by means of hyperbole, not as a factual description of the current situation. Sorry if it came across otherwise.

When it comes down to the git policy, I agree with you that we should allow assets licensed under terms that the FSF refuses to acknowledge as GPL-compatible if asset's license actually allows it. However, I must admit that I am uncomfortable with the current situation (where the official policy is that it is the FSF definition rules, and nobody in power seems to be willing to change that policy).

I am going on a three week vacation on Monday. I'll bring a a tablet to be able to go online, but will probably not spend much time with it, so don't expect much reviewing from men until August.

Code review for security advisory coverage applications

Group organizers

Group notifications

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