Signup Code

welcome,

As installation and usage instructions are quite important for us to review, please take a moment to make your project page follow the tips for a great project page. Also make sure your README.txt follows the guidelines for in-project documentation.

while waiting for an in-depht review of your module you can start out fixing some coding style issues detected by automated tools:
http://ventral.org/pareview/httpgitdrupalorgsandboxalbertliu1740336git

There is still a master branch, make sure to set the correct default branch: http://drupal.org/node/1659588 . Then remove the master branch, see also step 6 and 7 in http://drupal.org/node/1127732

We do really need more hands in the application queue and highly recommend to get a review bonus so we can come back to your application sooner.

regards

I'd propose to change the full name now

Changing status, see http://drupal.org/node/539608

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 :-)

Hello Albert.Liu

You module seems to be very interesting!

I welcome the change from sicode to Signup Code. So probably you will want to call it signup_code in the end. Hence, I would suggest you replace every sicode with signup_code
I don't know if that is a drupal standard, but it confuses me if all your functions and the database and the .module the .info and are not signup_code.

Otherwise the code looks solid to me.
Good luck

Hello
I don't know what you meant with the reference to cck. The cck module is called cck...

The point is if you use drush to download the module you would do something like this:

drush dl signup_code
drush en sicode

and if I want to have my module or feature depend on Signup Code drush wouldn't download it.

If you mean you only want to change the title and not the short project name then you could say so and make sure the git clone link in the original post reflects that. I think it is quite confusing otherwise. (on the sandbox page it suggests to clone it as signup_code now.)

Otherwise as I said before, I don't see any flaws.

The folder name of the module is called "signup_code" but the files inside are "sicode.extension".
This is really confusing and definitely not a good practice ! As the purpose of the Project Application is to promote good practices in my opinion this name problem should be solved before releasing the project application.

The folder name is artificially created for sandboxes from the project page label and cannot be set by module maintainers. The folder name for full projects is determined by the project short name, so just make sure that you use "sicode" for that once you promote it.

Any other application blockers?

Ah ok it's good to know. I'm checking again and I give the vetted promotion if I see nothing else.

There are some misspelling on the texts, obviously you are not a native English speaker so I can perfectly understand. ex: If checked then you can choose follow role to use owner feature. Maybe it means: if checked then you can allow thefollowing role to use the owned feature. Not really an issue but maybe some friend or some dictionnary could help to make the sentences "more english".

1. You don't need to mention a licence on the readme.txt. the LICENSE.txt is automatically added later.
Please remove all text mentioning a license.

## License

Copyright (c) 2012 [Albert Liu]

Licensed under the [GNU GENERAL PUBLIC LICENSE Version 2], you may not use
this file except in compliance with the License.

Unless required by applicable law or agreed to in writing, software distributed
under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR
CONDITIONS OF ANY KIND, either express or implied.  See the License for the
specific language governing permissions and limitations under the License.

[GNU GENERAL PUBLIC LICENSE Version 2]: http://www.gnu.org/licenses/gpl-2.0.html

Beside that the module works as expected and as there are no user entered input and the db queries are using correctly the placeholders I don't see any reason to block this module after the license text has been removed.

change status as the license issue is a blocker.

Since that paragraph does not state a different license than used on drupal.org (GPLv2) that is not a blocker.

License issues are only blockers if module authors try to state a different license or if they upload third party code.

Ok so if Klausi thinks that the GPL is not an issue, I don't see other reason to block this PA.

BUT...

I can advise to save into a variable the data instead of repeatedly calling a function that executes a dB query.
This kind of practice can slow down dramatically the loading speed of a page (specially when there are lot's of modules installed).
sicode.module line 102:
$has_owner = count(_sicode_get_owners());
Then again on line 113:

  if ($has_owner) {
    $owners = array(0 => t('- Please choose -')) + _sicode_get_owners();

Same double call to a function executing a query on lines 372 and 373.

But as optimization is not a blocker and I misunderstood the GPL issue, I guess this is PA ready.

Thanks for your contribution, Albert.Liu!

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.

status fixed