[D7] Selfi

Please add the below points in this issue about project description.

1. Git clone command: Please follow instructions:
You can find the correct git clone command for your sandbox by clicking on the Version control tab, removing the checkbox in front of "Maintainer", and clicking Show. You can then copy-paste the git clone command from the codeblock below "Setting up repository for the first time".
2. Please attach the zip file as feasible.

Assigned to myself for next review that may be tonight.
Please add more information on your project page.
As all reviewers 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).

Automated Review

Best practice issues identified by pareview.sh / drupalcs / coder are fine http://pareview.sh/pareview/httpgitdrupalorgsandboxdasgautam272437741git

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and/or fragmentation.

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

No: List of security issues identified.

1. (+)selfi_theme : you are passing the variables without sanitization in template.you need to sanitize it using check_plain.
2. (+)selfi_form_alter : you are passing the variables in drupal settings js without sanitization.you need to sanitize it using check_plain.

Coding style & Drupal API usage

1. Readme.txt is nice.
2. selfi_help : Use single quotes instead of double quotes as per drupal coding standards.It will also give some performance hits as well.Add some more information in hook_help
3. Move the admin configuration settings callback functions in seperate selfie.admin.inc file.It will give some performance hits as well.
4. selfi_theme : Change the template 'take-selfi' name with module name prefix.
5. (*)selfi_user_profile_submit: After viewing this module code it looks that module will work only for public file system ? if it is then please add a comment and also add on the project page regarding the same.
6. selfi_upload_pic : Please add comments above at various lines in code.
7. selfi_upload_pic :
   

     $status = array(
       'status' => ($success) ? array(
         'msg' => 'File saved successfully.',
         'value' => 1,
         'file' => $filename,
       ) : array('msg' => 'Problem while saving the file.', 'value' => 0),
     );

   Need to translate the message 'File saved successfully'

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

This review uses the Project Application Review Template.

Assigned to myself for next review that may be tonight.
I would recommend you, please help to review other project applications to get a review bonus back. This will put you on the high priority list, then git administrators will take a look at your project right away :)

Read (commit 215f909):

1. selfi_help: Avoid use of html tags in translatable string.
2. selfi_upload_pic : Don't use time() rather user REQUEST_TIME https://api.drupal.org/api/drupal/includes!bootstrap.inc/constant/REQUES...
3. selfi_webrtc_settings : Add a validation function as well to check the writeable permissions on that particular folder.Else add a hook_requirements to check that whether the selfi_clicks directory exits.
4. Please highlight on your project page that module only works with public filesystem.
5. selfi.tpl.php : Add the available variables in the above comments that will be benificial for all the users.

As all reviewers are currently quite busy 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 :-)

Assigning to myself for final review.

Automated Review

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

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

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.

Manual Review

Classes, namespaced to your module are better than generic CSS IDs.

Spacing in JS doesn't follow Drupal coding standards.

(+) Your JS behavior should take into account the context and settings that are passed in.

It looks like your JS could have most/all of the internal functions in the closure, and not on the Drupal object itself.

You should note that thse get stored in the public filesystem, and are therefore publically accessible.

You are plaining the width and height, but look into using drupal_attributes() instead.

No need to call exit() in selfi_upload_pic. One drupal_exit() is better, two the page callback system will do this for you.

(+) You should really have configurable filesize and dimension checks. You should probably also validate that it is actually a PNG.

(*) You have a menu callback that access $_POST directly w/o checking for a valid token. This is a security problem. See http://epiqo.com/en/all-your-pants-are-danger-csrf-explained and https://docs.acquia.com/articles/protecting-your-drupal-module-against-c...

(*) With the CRSF, a user could fill the disk with images, and crash the server.

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

Hello @das.gautam27,

Its a great module to upload pictures to Drupal's profile. Reminds me of popular social media sites. Great Job!
So, I had a high level look at your module's code and site functionality. My observations are as below,

1. You are creating a directory as "selfi_clicks" in the public files section. To do this you are using drupal_mkdir(). But why not removing the directory while the module is uninstalled? Since you are also removing all the variables from database keeping the files to server location doesn't make sense. To do this you might have to use drupal_rmdir() function.
2. Initially the directory is created by the name of "selfi_clicks" but then I tried to change the directory name from configuration. But even I changed the name it was not changed in sites/default/files directory. It remained as "selfi_clicks". Is it that I have to manually create the folder and then change the name? That wouldn't be good UX right? Even if you tell users to create the upload directory manually and then change the location what will happen if the previous directory had some images stored?
3. In selfi.module you have implemented hook_form_alter() but from the code I can see you have only altered a single form. So why not use hook_form_FORM_ID_alter? It's always better to use this one if you are not altering multiple forms in your module.

I hope these makes sense, I'd love to hear your thoughts on these above.
Thanks!

Wow, what a cool module!

Jut a few nitpicks:

  $items['upload-pic'] = array(

Menu paths should best be prefixed with the module name, especially one like this which AFAICT is purely internal.

$output .= '<p>' . t('It provide falicity to save user picture in public folder.<p> We are also working on providing facility to the user for their previous clicks clollection as a gallaery.') . '</p>';

A few typos in this line.

      'title' => t('Take Selfi'),
      'description' => t('User who can take selfi'),

Both strings could be a little clearer. Also, while the module is called 'selfi', the noun is 'a selfie' -- eg http://en.wikipedia.org/wiki/Selfie.

> * Implements hook_form_alter().

It's not this hook any more, it's hook_form_FORM_ID_alter() now.

I don't see a project at https://www.drupal.org/project/selfie ...

One thing I noticed is that the photo I take with the module looks squashed. Is that to do with the browser though?

Hi @Gautam,

Very unique idea you got. Great. Looking forward to use this in my projects. Please keep contributing with such cool stuff.

Automated review result:

FILE: /var/www/drupal-7-pareview/pareview_temp/selfi.module
---------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
---------------------------------------------------------------------------
148 | ERROR | [x] Closing parenthesis of array declaration must be on a
| | new line
---------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
---------------------------------------------------------------------------

Can you please check and fix this?

Manual review result:
Individual user account
Yes
No duplication
Yes
Master Branch
Yes
Licensing
Yes
3rd party assets/code
Yes
README.txt/README.md
Yes
Code long/complex enough for review
Yes : Code is long enough for review.
Secure code
Yes : Looks secured
Coding style & Drupal API usage
Yes : Looks good and follows drupal coding standards.

Thanks,
Omkar

Hi Gautam,
I installed, tested and ran through your code. Everything looks great!

Thanks for your contribution, Gautam!

I updated your account so you can 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 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.

Thanks to the dedicated reviewer(s) as well.