[D7] Per Role Picture

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxsiddharth892452719git

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.

Hey Siddarth,

I just cloned this project and enabled on local and tried uploading image on a certain role.

per_role_picture.admin.inc file:
1) Line 72 generates a fatal error.
DOException: SQLSTATE[42S22]: Column not found: 1054 Unknown column 'uid' in 'field list': INSERT INTO {per_role_picture} (rid, fid, uid) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2); Array ( [:db_insert_placeholder_0] => 6 [:db_insert_placeholder_1] => 17 [:db_insert_placeholder_2] => 1 ) in per_role_picture_form_submit() (line 72

You are trying to insert "uid" in $record -> which is later entered into per_role_picture schema.
But this column does not exist there.

Reference.

      $record = array(
        'fid' => $file_saved->fid,
        'uid' => $user->uid, // <strong>THIS THING</strong>
      );
      if ($file_saved->status === FILE_STATUS_PERMANENT) {
        db_merge('per_role_picture')->key(array('rid' => $rid))->fields($record)->execute();
        drupal_set_message(t('User picture for @name role has been saved.', array('@name' => $role->name)));
      }

Hi Guptas89,

Please find my findings:
- [Blocker] As noticed by kunal.kursija, it's currently impossible to upload a picture to a role due to the critical error.
- [Blocker] You should probably alter the form on admin/people/permissions/roles instead of creating a new one. If another module is enabled and altering this form, you basically by-pass it.
- [Blocker] Some functions are not following coding standard naming:

• role_picture_delete()
• user_picture()
• upload_picture_access()

For example, a future update of Drupal core may provide a user_picture() function in User module.

- Missing empty line between per_role_picture_help() and per_role_picture_menu() comment's.
- Some functions would be easier to read with some additional blank lines or comments (theme_per_role_picture_admin_roles(), per_role_picture_user_load(), theme_per_role_picture_role_order_settings() for example).
- I'm not sure

$filepath = $file->uri;

is very useful, you can use $file->uri directly.
- Typo mistake in "Submit hadler for set role weight." (~l 130 in per_role_picture.admin.inc).
- I'm not exactly sure what this line

<?php
$rows[] = array(array('data' => drupal_render($form['name']), 'colspan' => 2));
?>

is supposed to do (~l 173 in per_role_picture.admin.inc) but it's adding a wired blank line at the end of the table.
- The link "picture" in admin/people/permissions/roles may be easier to understand with "edit role picture's" or "edit picture".
- I haven't been able to upload a picture but I think it would be great to had image preview near the upload field (like image fields in node for example).

- [Q] Why do you provide a new admin interface to set the role order instead of using the existing weight field available on admin/people/permissions/roles ?

@Review

Looks good to me just one thing as your module is mostly (almost) depends on the user picture, you can use hook_requirements to check whether user picture is enabled or not. So that you dont have to write more access checks.

That sounds like an improvement but not an application blocker, anything else that you found or should this be RTBC instead?

@klausi - Yes

 role_picture_delete($rid);

This method is not defined so causing fatal error while removing image. Also, there should be an message when image is removed. And I think, this is kind of blocker.

changing status

Hi :)

in .module not use function t(), why?

Add function t() in description and titles please!

example:

.module
Line 39 'description' => 'Set order of role in which user picture will be display. If a user have multiple role then role with lesser weight will be on high priority for user picture.',

hi,

Manual Review

Individual user account - yes

No duplication - yes, there was no module available similar to this

Master Branch - yes

README.txt - yes, required information given

Code long/complex enough for review - yes

Coding style & Drupal API usage - yes

It looks RTBC to me

Hi guptas89,

Thanks for this wonderful module.
I started playing with your module on local setup (Drupal 7.34) and found a bug. Below are the steps to replicate
BUG : If the role pic is deleted from settings, the user pic (came from role pic) is still alive.
Step 1: Add a role pic to "authenticated user" role
Step 2: Visit any authenticated user(who has no user picture set), will have the role pic now (as per functionality)
Step 3: Go to user edit page and simply save it.
Step 4: Delete the role pic now.
Step 5: Now visit the corresponding authenticated user, the role pic still remains in his profile.

You need to handle differently when the user is edited and saved. As there are also some bug which happens after the user is edited and saved.

Hope this will help you more to make it as a stable release.

Changing Status

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: Followsthe licensing requirements.

3rd party assets/code

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

README.txt/README.md

No: Does not follow the guidelines for in-project documentation and/or the README Template.
Here is a summary of the preferred format:

• Headings in all caps.
• Headings underlined with ===/--- to the length of the heading, followed by a newline.
• Two lines prior to headings (except the first one).
• Bullets denoted by asterisks (*) with hanging indents.
• Numbered lists indented 4 spaces.
• Bulleted lists indented 1 space.
• Text manually word-wrapped within around 80 cols.

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

There are a few things pareview didn't like, but they are all about formatting or commenting.

	FILE: /var/www/drupal-7-pareview/pareview_temp/per_role_picture.admin.inc
	---------------------------------------------------------------------------
	FOUND 3 ERRORS AFFECTING 3 LINES
	---------------------------------------------------------------------------
	83 | ERROR | Doc comment short description must be on a single line,
	| | further text should be a separate paragraph
	161 | ERROR | If the line declaring an array spans longer than 80
	| | characters, each element should be broken into its own line
	178 | ERROR | If the line declaring an array spans longer than 80
	| | characters, each element should be broken into its own line
	---------------------------------------------------------------------------

	FILE: /var/www/drupal-7-pareview/pareview_temp/per_role_picture.module
	---------------------------------------------------------------------------
	FOUND 8 ERRORS AND 1 WARNING AFFECTING 8 LINES
	---------------------------------------------------------------------------
	84 | WARNING | Format should be "* Implements hook_foo().", "*
	| | Implements hook_foo_BAR_ID_bar() for xyz_bar().",, "*
	| | Implements hook_foo_BAR_ID_bar() for xyz-bar.html.twig.",
	| | or "* Implements hook_foo_BAR_ID_bar() for
	| | xyz-bar.tpl.php.".
	85 | ERROR | Doc comment short description must be on a single line,
	| | further text should be a separate paragraph
	115 | ERROR | Comment indentation error, expected only 1 spaces
	121 | ERROR | If the line declaring an array spans longer than 80
	| | characters, each element should be broken into its own
	| | line
	150 | ERROR | If the line declaring an array spans longer than 80
	| | characters, each element should be broken into its own
	| | line
	150 | ERROR | If the line declaring an array spans longer than 80
	| | characters, each element should be broken into its own
	| | line
	154 | ERROR | If the line declaring an array spans longer than 80
	| | characters, each element should be broken into its own
	| | line
	251 | ERROR | If the line declaring an array spans longer than 80
	| | characters, each element should be broken into its own
	| | line
	254 | ERROR | If the line declaring an array spans longer than 80
	| | characters, each element should be broken into its own
	| | line
	---------------------------------------------------------------------------
	

I experienced a blocker issue.

1. Blocker: Pictures aren't showing up. Steps to reproduce:
   Enable module - drush en -y per_role_picture
   Ensure user pictures enabled - drush vset user_pictures 1
   Edit picture for authenticated user role - admin/people/permissions/roles/picture/2
   Upload picture
   Save
   Visit My account.
   Picture not displayed. Alt text "admin's picture".
   Create new user "Jack" with role authenticated user.
   Log in with new user.
   Visit My account.
   Picture not displayed. Alt text "Jack's picture"
   Check role picture settings - admin/config/people/per-role-picture
   Picture missing. per_role_picture_preprocess_user_picture user->picture is always coming back null.

This review uses the Project Application Review Template.

This looks like a piece of cool functionality, but it isn't working in a clean D7.35 environment.

Now it looks good to me. Marking RTBC

manual review:

• why is per_role_picture_upload_picture_access() necessary? I think it should be enough to specify the permission in hook_menu().

But otherwise looks good to me.

Thanks for your contribution, guptas89!

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.