[D7] Who Logged In

Multiple Applications

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

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

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

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.

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.

Manual Review :
1. Module install properly.
2. Block(Who Logged In) created by module, get assinged in any region and information get display properly..
3. Module contain more then 5 hook_ function and more than 120 lines of code.

So, This is an very simple logic of displaying last logged in user.Code and functionality look good for me.

But some work needed in below area, if possible.
1. There is an confusion related block title. Default Block title should be "Who Logged In" instead of "Logged In Users" to avoid confusion.
2. If you provied link to the user page would we advantage of the module.Right now it display only user name as text.

Please change the git clone with: git clone --branch 7.x-1.x http://git.drupal.org/sandbox/albertski/2326193.git who_loggedin . A folder with the right name will be created then automatically.

Automated Review

pareview.sh founds only one warning as described in the task description above.

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

No: Does not follow the guidelines for in-project documentation and/or the README Template. The description in the project documentation and in the READMY file is very short.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity. But the module has a very simple logic.

Secure code

Yes: Meets the security requirements.

Coding style & Drupal API usage

1. The tag reference @return is missing in the function who_loggedin_list().
2. If no user has logged in since last x hours, the user list in the block is empty. A short info will be better e.g. "No user logged in.".

That are all minor points, anything else major that you found or should this be RTBC instead?

I haven't found major issues. This module works fine for me and is ready for RTBC.

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

• DrupalPractice has found some issues with your code, but could be false positives.
  

  
  FILE: /home/klausi/pareview_temp/who_loggedin.module
  ---------------------------------------------------------------------------
  FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
  ---------------------------------------------------------------------------
   122 | WARNING | Do not call theme functions directly, use
       |         | theme('item_list', ...) instead
  ---------------------------------------------------------------------------
  
  Time: 36ms; Memory: 4Mb
  

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

1. projec tpage is too short. What is the difference to the core who is online block? See also https://www.drupal.org/node/997024
2. theme_who_loggedin_list(): do not use l() to create the link to the user, use theme('username', ...) instead.
3. who_loggedin_settings_form(): you should use #element_validate with element_validate_integer_positive here to make sure you have a number, see https://api.drupal.org/api/drupal/developer!topics!forms_api_reference.h...
4. who_loggedin_list(): the query should exclude users that are blocked.
5. who_loggedin_list(): Do not use db_select() for simple static queries, use db_query() instead. See https://www.drupal.org/node/310075
6. who_loggedin_list(): so this will only show users that actually logged in in the last 24 hours. If I logged in 4 days ago having a session over several days and I'm active on that day I will not show in the list? Why don't you check the "access" DB field instead?

So the project page is a blocker right now. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

when cloning the folder name do who_loggedin

and I added the correct syntax to work with bd

and you first try

Manual review

A very simple module, I have spend an hour and notice few points as mention below.

i. Module allows me to save alphanumeric values for 'who_loggedin_hours' variable, it should allow only number.

ii. While rendering the account name use $account instead of $user for readability.

Hi albertski,

I have the updated code and it still takes alphanumeric character. i think the problem lies somewhere in the below condition

..
  if (!is_int(intval($element['#value'])) || $element['#value'] <= 0) {
...

the intval function typecast the variable and so for element value '24em' it returns '24' and hence it skips the character validation. Simply using the in_numeric function will solve the issue.

Git clone failed for http://git.drupal.org/sandbox/albertski/2326193.git while invoking http://pareview.sh/pareview/httpgitdrupalorgsandboxalbertski2326193git

Git clone failed. Aborting.

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

I have tried to clone your module, and it works.
When i read your module, you define a constant called WHO_LOGGED_HOURS. Can you tell me why you choose to define constant than do with variable_set function from drupal.
I still found issue with your code on http://pareview.sh/pareview/httpgitdrupalorgsandboxalbertski2326193git, you should use theme('item_list', ...) instead of theme_item_list()
How about if you make the WHO_LOGGED_HOURS configurable, its better i think, so user can set them

/**
 * @file
 * Who Logged is a baisc module for displaying who logged in.
 */

s/baisc/basic

...but that's a minor issue :) RTBC

Non-blocking issues:

• Use single quotes instead of double where possible for Drupal code standards and a very small performance benefit
• in SELECT uid, name, login, mail, access... it doesn't look like you need the login and mail columns?
• Instead of an admin form, consider using hook_block_configure() to allow admins to configure the block your module provides
• Be careful with is_numeric(), it will return true for floats, octal, hex and other unexpected values. Consider using ctype_digit()
• In who_loggedin_validate_hours(), is_int(intval($element['#value'])) will always return TRUE, because intval() will always return an int, so this condition is not a good check
• There's been some bad advice in this thread about how to validate that value, klausi's above suggestion is correct to use element_validate_integer_positive(). You don't need to define your own validation function to do this again.

Thanks for your contribution, albertski!

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.