[D7] Login tracker

Most of the code seems good to me, but:
1. Please follow the instructions of the Pareview: http://pareview.sh/pareview/httpgitdrupalorgsandboxleewillis772288283git...
For example you should add type of the @param in the comments, etc.
2. Please review other modules as it is recommended by the checklist:
https://drupal.org/node/1011698
https://drupal.org/node/1975228
if you like.

Hi leewillis77,

Overall impression

the overall impression of the module is very good.

Functionality

The logging itself, the exclusion via a permission and the views integration is working.

PAReview

Using PAReview.org I found a few minor coding/doc standard issues like missing full-stops. See the report here: http://pareview.sh/pareview/httpgitdrupalorgsandboxleewillis772288283git

I have attached a patch which should fix those issues.

Comments

You are using a permission to exclude users from the tracking - as the user-1 automatically has every permission, this user is always excluded from tracking. However, thanks to the hook_login_tracker_track_login_alter the behavior can easily be altered via a simple hook implementation, therefor i think it is OK. You could point this out in the README, however.

Another thing is that there are already two modules which implement very similar functionality - Do you think it is an option to cooperate with the maintainer of login_history on a 2.x version of login_history?

PS: I would need a review of my project-application over here: https://drupal.org/node/2288389

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.

As my concerns where fixed, I'll put this as RTBC.

I agree on your thoughts about the similar modules.

Multiple Applications

It appears that there have been multiple project applications opened under your username:

Project 1: https://www.drupal.org/node/2288335

Project 2: https://www.drupal.org/node/1609068

As successful completion of the project application process results in the applicant being granted the 'Create Full Projects' permission, there is no need to take multiple applications through the process. Once the first application has been successfully approved, then the applicant can promote other projects without review. Because of this, posting multiple applications is not necessary, and results in additional workload for reviewers ... which in turn results in longer wait times for everyone in the queue. With this in mind, your secondary applications have been marked as 'closed(duplicate)', with only one application left open (chosen at random).

If you prefer that we proceed through this review process with a different application than the one which was left open, then feel free to close the 'open' application as a duplicate, and re-open one of the project applications which had been closed.

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

Duplication

This sounds like a feature that should live in the existing Login History project. Module duplication and fragmentation is a huge problem on drupal.org and we prefer collaboration over competition. Please open an issue in the Login History issue queue to discuss what you need. You should also get in contact with the maintainer(s) to offer your help to move the project forward. If you cannot reach the maintainer(s) please follow the abandoned project process.

If that fails for whatever reason please get back to us and set this back to "needs review".

Please add the differences to the existing projects to the project page and follow https://drupal.org/node/997024

Automated Review

PAReview came up clean.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No multiple applications

Yes

No duplication

Yes (see below for details).

In #10, it is said the delta functionality can be implemented as a patch to Login History. The applicant, however, wants a lightweight module, and extending an existing one will not accomplish that. Personally, I don't think that duplication should not be regarded as a blocking issue if the module is otherwise useful and fulfils a specific need (her: lightweight and simple) that the similar module does not.

However, users need to be informed about possible functional overlap. This should be made is section discussing similar projects on the project page that:

1. acknowledges the existence of similar projects; and
2. briefly explain how they are different.

This requirement is fulfilled here, so that is why I am setting this to "Yes".

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements

3rd party code/content

Yes: Follows the guidelines for 3rd party code.

README.txt

Yes.

The project contains a markdown README.md instead of README.txt. Technically, this does not follow the guidelines for in-project documentation (which does not mention README.md or markdown formatting). But I think README.md and the markdown format has been allowed in the past, so we can't start regard this as a blocking issue now.

Code long/complex enough for review

No: The project is only 77 lines of PHP code (after stripping comments and blanks).
It is too simple to count as a real module, as it have no hook_menu, no configuration settings, no user input, and no user facing text output.

It does not follow the guidelines for project length and complexity.

However, it looks useful and well executed, and deserves to be promoted to a full project.
IMHO, a larger and more complex project should have been submitted for review for full git access, so I don't think this application is complex enough to lead to full git access. (But a git admin will make the final call on this.)

Secure code

Yes, it follows guidelines for writing secure code. It accepts data from client modules and that data can later be displayed by yet another client module. The data is inserted into the database in a secure way, and the way I read the guidelines, it is up do however renders this data to sanitize upon output for XSS exploits. (But this should be looked at again by a git admin.)

Coding style & Drupal API usage

Yes: It is a very small module, so there isn't much to check, but what there is, is good. Good use of comments and docblocks as well. It should be said that there is no hook_help(). It is good coding practice to have this hook for every enabled module, but this is not a blocker.

There are IMHO no more blockers. Moving to RTBC. Note that promotion will not happen until a git administrator has given this a second set of eyeballs.

Adding tag.

Automated Review

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

No issues.

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

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and fragmentation. Similarities adequately addressed.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements

3rd party code

Yes: Follows the guidelines for 3rd party code.

README.txt

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

Code long/complex enough for review

No: Doesn't meet the guidelines for project length and complexity.

Secure code

Yes.

Coding style & Drupal API usage

1. You should add the foreign key entries to login_tracker_schema().
2. (+) You need to implement hook_user_delete() to clean up your tables when a user is deleted.
3. How do you get at the data w/o Views? You need to have some sort of API call to get a user's login history.
4. You need a mechanism to either age tracking by date or number of logins (or both)
5. You need a mechanism to be able to delete a users login history or all login histories

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.

I am not seeing any blocking issues to do a single project promotion of this. However, it is not complex enough for vetted access.

I will investigate the other contributions to see if they are adequate for vetted access.

However, I don't really think this module is usable by a wider audience as-is. The module looks well written, and does things properly. The hook definitions you define are a good approach to the "store what you need", which gives people flexibility. Views integration is good. However, points 2-4 mentioned above are really needed to make this a complete module. If you can take care of these, then you will definitely meet the complexity requirements, and we can do vetted access after the new features have been reviewed.

Let me know how you want to proceed, but my preference would be the paragraph before this one.

I went ahead and promoted this to a full project. I will evaluate the patches you reference when I have a chance. I you rethink the suggestions I made, and implement these features, we can use login_tracker for vetted access...

Thanks for your contribution, leewillis77!

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.

I looked at the commits from the links above.

e_commerce_mailcheck is essentially a one hook module to a jQuery library. It has a dependency on Libraries, but doesn't really use the API (no hook_libraries(), no libraries_load(). The JS doesn't follow Drupal conventions. I cannot find any commits to mailcheck. This is something I would push back to Needs Work.

The other commits (and I admit I could have missed some) are one or two line patches for bugfixes, and not new/substantial features.

solr_boost_by_node_type looks like it has promise, but it is currently (commit dadb6e3) two functions, and about 90 lines.

The review process 'ensure[s] you are writing secure code, following the coding standards, generally following best practices, familiar with proper usage of the Drupal APIs, and promoting collaboration over competition (i.e., not duplicating functionality already available in other modules).' Part of this process is make sure reviews are looking at substantial enough code, which is where the five function / 120 line guideline comes from. This is not a strict rule, and admins do stray from this from time to time.

I am not seeing enough support to grant vetted access. The biggest area I am seeing a lack in is a demonstration of writing secure code in the context of a complete module, especially with handling user input. While we don't require all modules to handle user input, or interface with other APIs in a secure manner, I think this is a competency we would like to see demonstrated if we want to grant vetted access based on a lean module.

@leewillis77 I also encourage you to continue with your public contributions to the Drupal community, and to not be discouraged by the review process. When you have something ready for us to review again, we will take a look.

Just removing myself from this issue. I will still follow it, though, in case there is any activity.