[D7] Simplelogin

Git clone command for the sandbox is missing in the issue summary, please add it.

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

Hello satheeshkumar.six,

I ran your code against auto code sniffer at 'http://pareview.sh'. It gave 2 errors of which I think you should resolve the following :

File - css/simplelogin.css
error - 231 | ERROR | [ ] Style definitions must end with a semicolon

Fix the below issues:

1. You are using variable_get function in your module variable_get('simplelogin_fid', '') on line no.173. Create .install file and use variable_del('simplelogin_fid'); on hook_uninstall function
2. After I download your module on git. Your module folder look like 2640316. Please change your module folder name.
3. Describe detail about your module on module page.

Hi @satheeshkumar.six

There needs to be a validation message for Image upload if the uploaded extension is wrong.
For example: The selected file "apache2.conf" cannot be uploaded. Only files with the following extensions are allowed: png, gif, jpg, jpeg.

Currently it always seems to display the success message.

And @jaffaralia Regarding changing folder name, Is not it all sandbox modules behave like this?

Thanks

@satheeshkumar.six,

Even not mandatory, its good to have a message for wrong extensions.
See attached for reference.

Thanks

@satheeshkumar.six,

I just added a standard way of using file upload validation. That was not a screenshot of your module. I am asking you to implement like that if someone try to upload wrong extension.

For example: If someone tried to upload doc file, then it gives a error message that the file is of wrong extension.

Let me know if you need any clarity in this.

Thanks

Automated Review

I ran your code against auto code sniffer at 'http://pareview.sh/pareview/httpgitdrupalorgsandboxsatheeshkumarr2640316git'.

Review of the 7.x-1.x branch (commit 8894019):

Coder Sniffer has found some issues with your code (please check the Drupal coding standards).
FILE: /var/www/drupal-7-pareview/pareview_temp/simplelogin.install
--------------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
--------------------------------------------------------------------------
6 | ERROR | [ ] There must be no blank lines after the function comment
10 | ERROR | [x] Expected 1 newline at end of file; 0 found
--------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------

Time: 170ms; Memory: 7.75Mb

No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

Note that perfect adherence to Drupal Coding Standard is NOT a reason to block an application, except for total disregard of them. However, modules should follow them as closely as possible.

Manual Review

minor coding standard errors are surely not application blockers, please do a real manual review.

@satheeshkumar.six,

Module seems to quite good. Just recommendation, it would be good if you mention more about module in README file.

The URL /admin/config/simplelogin currently returns a whitepage and the error "Warning: call_user_func_array() expects parameter 1 to be a valid callback, function 'system_admin_menu_block_page' not found or invalid function name in menu_execute_active_handler()". Change the menu item to also include the required file like this:

$items['admin/config/simplelogin'] = array(
  'title' => 'Simple Login',
  'description' => 'Configuration of simple login image',
  'position' => 'right',
  'weight' => 3,
  'page callback' => 'system_admin_menu_block_page',
  'access arguments' => array('administer site configuration'),
  'file' => 'system.admin.inc',
  'file path' => drupal_get_path('module', 'system'),
);

simplelogin.module
Line 66: Place Login to Account inside a t()
Line 134: Place Uploading inside a t()

templates/page--simplelogin.tpl.php
Line 17: Rename Login-In to Login
Line 34: Place Forgot password? inside a t()

You might also want to move the background image css logic from your template file to the hook_preprocess_page, which is already present inside your module file.

Hi @satheeshkumar.six,

Scenario: If someone tries to upload doc file, then it should give a error message that the file is of wrong extension like in the attached screenshot. Its good to have.

Let me know if you need any more clarity.

Automated Review

Looks good. No test cases, but not an application blocker and not really essential for a simple module such as this.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Does not appear to cause module duplication and/or fragmentation, although personally I'd probably aim to make a change like this in a custom theme.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements.

3rd party assets/code

Yes: Follows the guidelines for 3rd party assets/code.

README.txt/README.md

Yes: Follows the guidelines for in-project documentation and/or the README Template.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes: Meets the security requirements.

Coding style & Drupal API usage

1. Just a recommendation: Most of the .module file is hooks, but I'd move what you can (mostly the admin form) to a separate .inc file to reduce the amount of code loaded on each page request.
2. Just a recommendation: I'd rename your theme_sl_imageupload() function to theme_simplelogin_imageupload() (and update the references obviously) for consistency and (I think) coding standards.
3. Just a recommendation: You go to the effort of setting a progress indicator and message when uploading a background image but then remove the upload button so these won't be used.
4. Just a recommendation: You include a folder 2640316 in your repository that serves no purpose and can be deleted.

Generally shows a good understanding of the Drupal API and coding standards.

This review uses the Project Application Review Template.

Happy to set to RTBC. Let's hope the backlog of these get a final quick review and approval by a git administrator soon...

Changes look good to me.

Removing review bonus tag, you have not listed any manual reviews in the issue summary? Make sure to read https://www.drupal.org/node/1975228 again.

seems variable_get and drupal_get_path_alias function callback made on the tpl file, so try to avoid those on the tpl and instead have those as a variable on the module using theme preprocessor hook and use it on the tpl file.

@satheeshkumar.six
Location     
/simplelogin/simplelogin.module   

Problem synopsis      
Missing 'break' statement (at line 72)

just want to know that this module does not have help & permission hook. Do you want or you don’t want.

if not than why.?

Hi satheeshkumar.six ,

Module looks good. Some suggestion are :

1: Please update the path admin/build/modules to admin/modules in README.txt
2: Use a blank line just after <?php in simplelogin.inc and simplelogin.module

 <?php
 /**
  * @file

to

 <?php
+
 /**
  * @file

@visabhishek: Above are not blockers. Please change the status. Is this now RTBC after your review or are there other application blockers left?

@naveenvalecha : No i have never find other blockers. Now we can mark it as RTBC.

Thanks for your contribution! Congratulations, you are now a vetted Git user. You can promote this to a full project.

When you create new projects (typically as a sandbox to start) you can then promote them to 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 (here you get to ask fans of your project to help you!)

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and stay 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.