tablebooker

Hi,

Project URL?

seems like you still have to much work on it.

> README.txt is missing

further errors and warnings can be found here. http://ventral.org/pareview/httpgitdrupalorgsandboxjohannesdr1854260git

i have read your .module file line by line.every thing is ok just in lines 16 and 25 there is blank line that you can remove them from the code
in the .info file isn't that better that you define a package for you module ?

We are currently quite busy with all the project applications and I can only review projects with a review bonus. Please help me reviewing and I'll take a look at your project right away :-)

Priority is normal right now, see http://drupal.org/node/539608

Thanks for your contribution to the drupal community. Your code looks good I found one issue in manual review it is a minor issue please try to fix it.

Manual Review:

1) Do not generate HTML by yourself use l() function on line no. 106 in tablebooker.module file

Manual Review:

1. tablebooker_reservation_block_view(): The output of this function is vulnerable to XSS exploits. You need to sanitize data before printing it. see http://drupal.org/node/28984

Everything looks good to me.

http://groups.drupal.org/node/142454

We are currently quite busy with all the project applications and I can only review projects with a review bonus. Please help me reviewing and I'll take a look at your project right away :-)

A few comments:

tablebooker_perm()

hook_perm() was the Drupal 6 signature. In Drupal 7, use hook_permissions().

Also, permissions should typically start with a verb describing what the permission grants access to.

You don't actually use the permission that was defined anywhere in the module (which is why the incorrect function signature was not picked up).

This doesn't affect the module currently (other than the fact that there is no access restrictions on the functionality) ... and I don't see any other issues. So ... either remove the hook_perm or fix it before generating your first release. :D

Thanks for your contribution, johannesdr!

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:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

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 reviewer(s) as well.

johannesdr, I apologize, but I have rolled back this approval.

A project should demonstrate sufficient hook usage and/or API integration. You can find more info at Working with the Drupal API.

This is an important criterion so that code integrates well and can be improved over time.

I encourage you to continue developing and gaining from the feedback available in the git approval process.

Thank you for you contributions and understanding.

Re-opening this for discussion

If we do not approve a user we at least promote the project manually for them.

@mitchell: I disagree, this module implements plenty of Drupal hooks and I think it is sufficient for approval.

But there are some blockers anyway:

1. tablebooker_menu(): used permission does not exist.
2. tablebooker_perm(): that hooks does not exist in Drupal 7, see hook_permission().
3. tablebooker_perm(): permission is never used.
4. "variable_get('tablebooker_reservation_form_theme', 'light')": variables are never removed in hook_uninstall(), which is bad for Drupal's memory footprint since all variables are cached in memory.

but I have rolled back this approval.

A bit of feedback on process: Something as drastic as reversing an approval should be discussed before acting on. At least getting a thumbs up in irc if not posting a comment and waiting 24 hours.

@johannesdr - you're so close, keep it going ;)

@johannesdr, @klausi, & @greggles: Thank you for keeping the issue open; I didn't mean to suggest we cut short the dialogue or the quest for johannesdr's achievement.

klausi's solution of promoting this project, one-time, is great for johannesdr's needs and for contrib.

To explain this action further, I did this, even unfortunately stepping on peoples' toes, because there was a circumstance in the recent past where a user was approved with a block and an iframe, then he went on to create a bunch of very poor quality, paid projects, for which he's now being called out.

This measure, that a project meet a minimum level of complexity, is now in the Core Review Templates, and I sincerely believe it will help.

@johannesdr, I hope you don't feel as though I'm singling you out. I sincerely try to do right by my peers and act kindly in the project application issue queue. If there's anything else I can do for yor application, I'm happy to help.

@jthorson, I apologize for stepping on your toes. If you search by my participated issues, you'll find that I've done my best to expedite this process and make it more welcoming to applicants.

@mitchell: In this case, there is plenty of additional code in johannesdr's other sandboxes to justify promotion ... no, it's not 100% perfect, but there's another thousand lines of code there demonstrating (in my opinion) more than sufficient competence with the API to move this forward. This was a major factor in my decision to promote the project.

The other major factor is that this application sat in the queue for 12 weeks, despite having been marked RTBC. With that much delay in the RTBC queue, bumping it back to 'needs work' is paramount to sentencing jonhannesdr to another couple of months of waiting before being granted the 'git vetted' user status.

Maybe only those who have been through this process themselves truly understand just how demotivating that waiting period is ... it damn near drove me out of the community before I even started. Rather than inflicting that pain on yet another applicant, I chose instead to point out the remaining concerns (yes, I missed the variable_del()) and approve the application.

In my opinion, reversing that approval on the basis that projects must meet a minimum level of complexity is doing a great disservice to johannesdr. Not only was his other code taken into consideration before the approval, but the entire 'code must meet minimum length' standard was implemented without a strong consensus in the g.d.o thread (other than it being a 'guideline'), and the 'code must meet minimum complexity' tweak was added without any discussion at all. Yes, Drupal is a do-ocracy ... but this sort of unilateral process change does NOT occur without discussion, and SHOULD NOT occur without consensus.

For the record, I re-approved johannesdr as soon as I had noticed the permission had been removed. If we need to have further discussion on the topic, then so be it ... but I trust him to take the feedback into account and fix the identified issues before packaging a release, and we should not make him wait while we work to sort out our own dirty laundry.

Yes, Drupal is a do-ocracy