[D7] Account Author

Hi Kapil,

I have cloned your module using:

git clone --branch master http://git.drupal.org/sandbox/Kirschbaum/2170431.git user_auth_log

However, it seems there are several issues in terms of your module's file structure. I am not sure if perhaps files have been lost through commits or perhaps something else, however when cloned the module folder contains .idea and .git, which means that the module can not be installed to a Drupal site.

please see: https://drupal.org/node/1074360

Also, for proper Git usage, please see: https://drupal.org/node/803746

If you need any help, please ket me know and I'll help as best as I can.

Cheers,
Adam

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.

Hi Kapil,

Here are some quick issues to get started:

- All functions should be prefixed with your module name.
- Got some spelling mistake.

Also, please take a look at the automated code review results here:
http://pareview.sh/pareview/httpgitdrupalorgsandboxkirschbaum2170431git

Thanks
Shantanu

Hi Kapil,

1. You should delete your master branch, according to https://drupal.org/empty-git-master.
2. Branch doesn't match the release branch pattern. It should be 7.x-1.x.

Thanks
Shantanu

Hi Kapil,

Glad that you are contributing to the community! I'm just starting to contribute myself and there is definitely a bit of a learning curve, but totally worth it. I'll keep an eye on this thread and help out where I can. For what it's worth I was able to download your module without issue, but I'm actually only seeing the .info file and nothing else.

Also, I wouldn't worry about the user_authlog module. It looks like shantanu2683 posted the incorrect link.

Hi Kapil,
This is good module. I have review this module and find there is missing argument 2 in variable_get function at line
227: $to = variable_get('user_authlog_settings');

Thanks,
Yash

Hi kapil.ropalekar,

No problem. I've taken another look at your module and have a few more comments:

• I'm seeing an extra (empty) folder called "account_author" inside the module directory.
• When I enable the module I get the following:
  [22-Jan-2014 06:31:12] PHP Parse error: syntax error, unexpected T_STRING, expecting ')' in /Applications/MAMP/htdocs/devel.loc/sites/all/modules/account_author/account_author.install on line 18
• I'm noticing that the .install file formatting is a little off. Have you tried to run your module through the Coder module or pareview.sh? http://pareview.sh/pareview/httpgitdrupalorgsandboxkapilropalekar2175715...
• I'm noticing that the .install file has a closing PHP tag "?>". In Drupal this is left out as a best practice: http://goo.gl/WE8cHU

Hope that helps!

Hi kapil.ropalekar,

I was able to download and enable the module without issue this time. It seems to function as described. I don't have any any additional comments about the code. Perhaps some of the more advanced reviewers will have some additional comments.

Good luck!

If you are confident that this project is ready please set it to rtbc next time. Thanks!

This tiny project has been untouched for five months, so it is time to give it a full review.

Automated Review

The automated review of your project by PAReview has found some issues with your code. As coding standards make sure projects are coded in a consistent style we ask you to please have a look at the report and try to fix them. While these are all trivial errors, things like wrong indents makes your code harder to review.

Manual Review

Individual user account

Yes: kapil.ropalekar follows the guidelines for individual user accounts.

No duplication

Yes

Master Branch

Yes: Follows the guidelines for master branch.

Git Clone Command

No. There is no git clone command in your issue summary.

Licensing

Yes: Follows the licensing requirements

3rd party code/content

Yes: Follows the guidelines for 3rd party code.

Project page

I find the name of the project ("Account Author") odd, as accounts are not authored, but created. I think "Account Creator" would be more descriptive.
Also. Please take a moment to make your project page follow tips for a great project page. It should argue the use case for this project better..

README.txt

Yes. While does not follow the guidelines for in-project documentation and the README.txt Template, it most of the required infirmation. It would be nice if it told users how users created before the module was created is handled, how users that create themselves are handled, and how users created by the super admin is handled.

Code long/complex enough for review

No: It consists enable and unistall hooks (27 lines) plus a single function (7 lines), giving a total of 34 lines of PHP code (after stripping comments and blanks). It creates a field (field_author_uid) for the user entity if this field does not already exist when the module is enabled, and deletes it when the module is uninstalled.
It then goes on to implement hook_user_insert() to insert the uid of creator in this field when a new user is created. The use case is to keep track of creators of users.
It is too simple to count as a real module, as it have no tables on its own, no configuration settings and no user input.
It does not follow the guidelines for project length and complexity.
However, it looks useful, and IMHO deserves to be promoted to a full project provided it is cleaned up a bit.
However, a larger and more complex project should have been submitted for review to apply for the "Create Full Projects" permission. (But I shall leave it to a git admin to make the final call on this.)

Secure code

Yes: Follows guidelines for writing secure code. (But being so tiny, there it not much to review).

Coding style & Drupal API usage

My notes:

• (*) The field is created by hook_enable() and erased by hook_uninstall(). While this works, it is unconventional. The symmetric solution where field is created by hook_install() and erased by hook_uninstall() should be used instead..
• (*) Breaks coding standards (see Automated review).
• (*) Give the project a more meningful title (see Project page).

For such a tiny module, I think the starred items (*) warrant going back to Needs Work. The rest of the comments in the code walkthrough are recommendations. I would like to read the code again when it is formatted properly.

Added tag.

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

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

Hello Kapil,

Your module looks good. Please check following "Automated Review" and try to resolve them.

See http://pareview.sh/pareview/httpgitdrupalorgsandboxkapilropalekar2175715...

Thanks,

let's keep this closed until we hear back from the applicant.

@ashwin.shaharkar: please review issues that are in the "needs review" state, thanks!

Please don't assign the review task to yourself. Assigning an issue to yourself is problematic for several reasons. If you want to contribute to the Drupal community, you need to learn the community conventions - including those about assigning ownership to issues.

Ok, now for the review.

First: The code now passes automatic robot review. This is good thing.

Two years ago, I wrote:

This is very small and trivial project that consists enable and uninstall hooks (27 lines) plus a single function (7 lines), giving a total of 34 lines of PHP code (after stripping comments and blanks). It creates a field (field_author_uid) for the user entity if this field does not already exist when the module is enabled, and deletes it when the module is uninstalled. It then goes on to implement hook_user_insert() to insert the uid of creator in this field when a new user is created. The use case is to keep track of creators of users.

It is too simple to count as a real module, as it have no tables on its own, no configuration settings and no user input. Hence this is tagged "PAReview: Single project promote".

However, this time around I would like to add:

The new field is named field_author_uid. This is an unfortunate choice because it is generic and may create name collisions with other modules that also would like to have a field_author_uid. The Drupal convention for function names is that names should include the project's machine name to harden them against name collisions. I believe best Drupal practice is also to do this with field names defined by contributed modules.

Provided the module is properly named "Account creator" (see below), the field it defines should be named field_account_creator_uid. This also makes it clearer what the field is used for (i.e. to store the uids of account creators).

In #18, I suggested that the project was renamed "Account creator", to better communicate its purpose. The author has changed the name of the sandbox to "Account creator" and the git clone instructions also clones a project of that name. However, the .info, .module and .install files are all named content_author. Having project file names and function names that do not follow Drupal naming conventions occupies two name spaces where one would do, and may even disrupt another project if somebody (after checking if there exist a project with the machine name content_author and not finding any) mistakingly believes that this namespace is unused. I think it is important for a project to be promoted, that its author demonstrates that he or she cares about coding standards and some basic conventions for module Drupal development. Because the Drupal project involves thousands of developers, each developer need to take care to avoid name collisions and other disruptions of other developer's projects.

Coding style & Drupal API usage:

• (*) Make sure the project has a human name that communicates its purpose.
• (*) Make sure the project has a machine name that matches the human name.
• (*) Make sure the project's machine name is used in the names of files, functions and fields according to best Drupal practices.

The starred items (*) warrant going back to Needs Work.

Thank you adding hook_help.

All the issue raised in #26 have been fixed.

Just a few nits:

• You keep referring to the "CCK" in your documentation for the module, but your module does not use CCK fields. It uses the core Field API (which is the correct thing to use for Drupal 7). I find the references to "CCK" in the documentation somewhat anachronistic.
• Also, the past tense of "arise" is "arose", not "arissen" (arise conjugation). In hook_help, instead of "Thus arissed the need for this module", I would suggest the following sentence "Hence the need for this module arose".

However, these are not blockers, so I am moving this forward to RTBC. Please note that promotion to full project will not happen until a Git administrator have given this a second set of eyeballs.

@kapil.ropalekar you might want to review some other projects in order to get the review bonus.

See http://drupal.org/node/1975228

Removing review bonus tag, you have not done any manual review, you just posted the output of an automated review tool. Make sure to read through the source code of the other projects, as requested on the review bonus page.

@kapil.ropalekar - please review and comment on the code in the module.

You can use this template to check for some of the key things we need to watch out for...

https://groups.drupal.org/node/427683

You can also test the module with simplytest.me
Eg like this http://simplytest.me/project/2562355 <= use the sandbox ID number.

When you have done those reviews, paste links to the reviews into your Issue Summary here.

Good Luck! And thank you for helping us test new projects!

Sorry that this relatively straightforward module application has been so long in the queue.

One reason for this to happen is that the Project name and the Project description itself are somewhat unclear, and hard to puzzle out...
If you are committing to providing a useful utility to the Drupal project library, then tips for a good project page suggest that you should clearly describe and sell why someone would want to use this module. Your project page should clearly describe some use-cases and examples, and maybe keywords that would help someone who was looking for a tool like this to find it. Think like a site-builder - how would they find your module, and why would they choose to use it?.

For similar reasons, it's hard for module reviewers to look at your module in a list and decide it was worthwhile reviewing, or what they would expect if they spent time doing so! Modules that are opaque in their purpose, provide no clear benefit, or do not provide sufficient testing instructions to allow someone to evaluate them are likely to get passed over in favour of more potentially useful ones..

(Heck, I'm only looking at it today because the most interesting thig about it is that it's at the bottom of the queue by date)

On inspection, I see that you have somewhat more interesting and useful help in your hook_help! Go ahead and copy that to your project page.

---------------------

Looking at the module itself ...
I'm afraid it's pretty sloppy. There are some immediate issues just from a quick scan.

In account_creator_enable(), you do check to see if the field_base was created, but not if the field_instance was. These are separate logical checks.

If an editor were to simply delete the field instance in field UI, /admin/config/people/accounts/fields/field_account_creator_uid/delete (but leave the field_base behind), then this logic would no longer work as expected. Sure, you don't *expect* an user to do that, but OTOH, you should not fail without warning.

If you were to take responsibility for creating and deleting fields - then you have to take responsibility for doing that cleanly.

Also - in field_create_field(), you have used the field_name 'field_account_creator_uid' - which has the prefix 'field_'.
Although it's not clearly documented, modules that provide their own custom fields, use their own module name as a prefix on the field_name. The core 'field' module provides field names that are prefixed with 'field_'. That's what is happening there. I understand why you copied it, and there are some tutorials out there that make the same mistake, but there is some understanding needed here.
gisle already described this issue in #26 above.

Your custom field is provided by your module, so the machine-name for it should be just

      'field_name' => 'account_creator_uid',

That prevents it from being confused with a user-managed field that field_ui added.

You have the creator account managed as 'text' - when clearly the only valid value there would be 'integer'.

... Or not, as it looks like anyone with the totally normal ability to 'manage own account' now has edit rights on this field, and is able to change the value of this field and therefore re-assign their own master account, or enter invalid data there! Surely this is not appropriate for the use-cases that this utility would be appropriate for?

The account creator shows up unfiltered on public user account pages - I would say that this would be expected to be undesirable behaviour in almost all use cases. It's easily fixed afterwards, but is bad default behaviour.

And before I even finish .. the line in
account_creator_user_insert()

  $edit['field_account_creator_uid'][LANGUAGE_NONE][0]['value'] = $user->uid;

doesn't use the correct entity_metadata_wrapper() approach for data manipulation.
A nice tutorial on entity_metadata_wrapper() is here

.. :-(

Given the high frequency of obvious Drupal-API problems in such a remarkably small sample of code - this review cannot pass.
It took me longer to point out all the things wrong with this version than it would take someone else to write a better version.

It seems that a site-builder would be able to get the same result as this module by just thinking about the problem and using a single Rules action to do this job powerfully.

On User Create, Set Account Creator field value to [global:user:uid]

For all the above reasons, I've got to put this application down again.
Sorry, but the problems outweigh the point of the module so hard it's pretty severe.
It's a little crazy that it's been in the queue so long only to get to this. But it's not been promoted - because it's not promote-able.

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

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