[D7] User activity log

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.

Add appropriate project application reviews. Please check project application reviews doc.

module gives me an error after enabling :(

I think user_activity_log_total_comments_by_user() giving an error when there is no comment added by login user.

yay , Find out the problem :) When comments module is disabled then it is throwing the site to an error page. If you are showing the total number of comments in block then you have to add the dependencies of comments module. Otherwise it will gives an error when comments modules is disabled.

Hi, thanks for the contribution. Let me just quickly list some things I found while going through the module:

The problem I see with this module is that the functionality can be easily duplicated with a views block. This module provides a static, non-configurable block. If you intend to create a plug-and-play solution to displaying user info in a block, you should provide a way to configure the output via UI. Otherwise I don't see the point of using this instead of views.

.info
- Please fix the description in the .info file from 'login' to 'logged in' and add a full stop.
- The description should mention, that the module provides a block, alternatively add hook_help.

.module
- Lines 51-54 and 115: The strings should be passed through t().
- Lines 70, 86, 104-105, 131: No need to set the third parameter in the query condition method if it is '='.

Module functionality:
- I tested it on the site where hundreds of thousands of nodes have been created by user 1 and all these nodes where displayed in the block... Please check the limit.

Please add a review bonus to speedup the process.

@chanderbhushan

1. Please follow README TEMPLATE.
2. There are some typos in .info file and also in variable names in your .module file.

Automated Review

FILE: /var/www/drupal-7-pareview/pareview_temp/user_activity_log.module
---------------------------------------------------------------------------
FOUND 3 ERRORS AFFECTING 2 LINES
---------------------------------------------------------------------------
51 | ERROR | [ ] Concatenating translatable strings is not allowed, use
| | placeholders instead and only one string literal
52 | ERROR | [ ] Concatenating translatable strings is not allowed, use
| | placeholders instead and only one string literal
52 | ERROR | [x] Concat operator must be surrounded by a single space

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 README Template.

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

• Please take a look at the README Template linked above and modify README.md accordingly.
• (*) Line 51-52: please take a look at how t() works and use named placeholders.
• (*) Line 54: use t() on this line too.
• Though not a requirement, it's frequently suggested that hook_help() should be implemented.
• (*) Enabling the block killed killed my site:
  PDOException: SQLSTATE[42000]: Syntax error or access violation: 1140 In aggregated query without GROUP BY, expression #1 of SELECT list contains nonaggregated column 'd7.n.nid'; this is incompatible with sql_mode=only_full_group_by: SELECT n.nid AS nid, COUNT(1) AS count FROM {node} n INNER JOIN {users} u ON n.uid = u.uid WHERE (n.uid = :db_condition_placeholder_0) AND (n.status = :db_condition_placeholder_1) ; Array ( [:db_condition_placeholder_0] => 1 [:db_condition_placeholder_1] => 1 ) in user_activity_log_total_node_created_by_user() (line 73 of E:\web\drupal7\sites\all\modules\user_activity_log\user_activity_log.module).

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.

This review uses the Project Application Review Template.

i have tested and it is working fine for me.
great work

There are two points :
1. use list instead of lsit when you name your variable is better.
2. In order to improve performance, I suppose you can use drupal cache api.

That's all, Thanks.

Good work Chander !

Great module.. This is very helpful to me. Nice work @chanderbhushan.

Hi;

I've completed a review of the project - findings below. Setting back to "Needs work" accordingly.

Automated Review

Completed - wthout any issues.

Manual Review

Individual user account

Follows the guidelines for individual user accounts.

No duplication

Does not cause module duplication and/or fragmentation.

Master Branch

Follows the guidelines for master branch.

Licensing

Follows the licensing requirements.

3rd party assets/code

Follows the guidelines for 3rd party assets/code.

README.txt/README.md

The README.md file is not in markdown format. This should either be README.txt - or rewritten to markdown format.

Code long/complex enough for review

Yes - although only just. The module is 130 lines long including comments & whitespace.

Secure code

Does not meet the requirements. The code fails to adequately escape node titles when outputting them in user_activity_log_latest_comment_by_user() and user_activity_log_latest_node_created_by_user().

Coding style & Drupal API usage

There are a couple of major-ish issues (including the blocking security one) and some more minor issues:

• (*) Data needs adequately escaping before output
• (+) Links should be created using l() rather than hard-coded HTML
• Minor typo in .info file "loged" should be "logged"
• "Total node created (18)" - should be "Total nodes created (18)"
• "Total Comments" should be "Total comments"
• user_activity_log_block_content() should exit at the top of the function if UID is 0, not the end
• Not sure why you're defining USER_ACTIVITY_LOG_USER_COMMENT_PUBLISHED, USER_ACTIVITY_LOG_USER_NODE_PUBLISHED - just use COMMENT_PUBLISHED and NODE_PUBLISHED ?

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

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

Hi - thanks for the quick updates. Some comments below:

Data needs adequately escaping before output.

This is now double-escaped, and you're using t() where you shouldn't be. So, in
l(t(check_plain($node_title)), $path)

t() is incorrect - it should only be called with fixed strings, not variables. It's also not adding anything here. The check_plain() that you've added is also incorrect since you're now using l() to construct the link. l() will sanitise the string for you. So - this should just be:

l($node_title, $path)

Not sure why you're defining USER_ACTIVITY_LOG_USER_COMMENT_PUBLISHED, USER_ACTIVITY_LOG_USER_NODE_PUBLISHED - just use COMMENT_PUBLISHED and NODE_PUBLISHED :- i have this because of http://pareview.sh/ suggestions.

Not sure what you mean here? COMMENT_PUBLISHED and NODE_PUBLISHED are already defined for you and available for use. Can you clarify why you're redefining your own constants here?

Hi;

Thanks - the module works as expected now. I'm happy with this. Probably needs another set of eyes to move it on.

Hi Chander,

I just found the module simply and useful, but there are some Drupal standard which probably was not taken in account.
1. The 'User Activity Log' block should be implement by using '#theme' so that it can be override in the theme and designed accordingly.
2. There should be check for empty results in user_activity_log_latest_comment_by_user function at line # 93. otherwise it will log notices in the watchdog.
3. Same is recommended for user_activity_log_latest_node_created_by_user function
4. Its not need to fetch the path alias separately as it is handled by l function itself.
5. you have defined $base_url; but have not used anywhere

Apart from this there are some typo errors
1. At line # 97 the Commented on with extra leading space ( May create issue while adding a translation from translate interface)

Tested on local machine code is working fine for me.

Thanks

@chander

Please use template file to render block content using hook_theme() to make it helpful for theme developers.

@chander
Now it seems fine.

Some issues found by automated review http://pareview.sh/pareview/httpgitdrupalorgsandboxchander1232573995git . See if it can be resolvable.

Not found any application blocker. Assigning it to Klausi for final review.

@Klausi I have found user_activity module. but it has not D7 release. See if it create a duplication issue. Otherwise code looks good to me.

@chander Good work !! Thanks for the contribution.

manual review:

1. project page: what is the use case of this module, what problem does it solve? Who sees the activity log? The user themselves or other users or the admin? See also https://www.drupal.org/node/997024
2. user_activity_log_latest_comment_by_user(): why the complicates string concatenation with "-"? You could just return a nested array with path and title as elements.
3. user_activity_log_theme(): the variables your template accepts are missing, see https://api.drupal.org/api/drupal/modules!system!system.api.php/function...
4. user_activity_log_latest_comment_by_user(): the "node_access" query tag is missing here, same for user_activity_log_total_node_created_by_user(). See https://www.drupal.org/dynamic-queries . Not sure if this should be a security blocker or not - it is kind of an edge case that a user is denied access to their own nodes where they are the author. Should still be fixed before we approve this.

Automated Review

I ran your code against auto code sniffer at 'http://pareview.sh/pareview/httpgitdrupalorgsandboxchander1232573995git'.

Review of the 7.x-1.x branch (commit 7237f2f):

Coder Sniffer has found some issues with your code (please check the Drupal coding standards).
FILE: /var/www/drupal-7-pareview/pareview_temp/user_activity_log.module
---------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
---------------------------------------------------------------------------
140 | 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.".
---------------------------------------------------------------------------

Time: 90ms; Memory: 6.25Mb

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.

Note that perfect adherence to Drupal Coding Standard is NOT a reason to block an application, except for total disregard of them. However, modules should follow them as closely as possible.

Manual Review

1. Lots of other module available in Drupal 7 like
   Heartbeat, Activity Log, please let us know how your module different than other modules
2. How can we use your module. Please mention in Readme file.

I am also with #55 Automated review should clear and Readme.txt file help lot to understand functionality.

Removing review bonus tag, you have not done all manual reviews, you just posted the output of an automated review tool or made comments about git clone commands. Make sure to read through the source code of the other projects, as requested on the review bonus page.

manual review:

1. you have not updated your project page, this is the place people see first when they come to your module. what is the use case of this module, what problem does it solve? Who sees the activity log? The user themselves or other users or the admin? See also https://www.drupal.org/node/997024
2. What are the differences to existing modules? Please add that to the project page so that users can make an educated choice.
3. user_activity_log_block_content(): do not call theme() here, return a render array instead. Drupal core will render it later for you. See https://www.drupal.org/node/930760
4. user_activity_log_total_node_created_by_user(): node_access tag is missing here. This is not a security issue, just a standard bug where the user might get the wrong number of nodes reported if a node access module is active.
5. user_activity_log_theme(): the variables for your template are still not defined. This should have 'total_node' and others as separate variables. See https://api.drupal.org/api/drupal/modules!system!system.api.php/function...

But otherwise looks goos to me. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Assigning to er.pushpinderrana as he might have time to take a final look at this.

Automated Review

Best practice issues identified by pareview.sh / drupalcs / coder. None

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

• 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

Most of the recommendations are already raised in previous comment, you may observe duplicate in my review but it would help you to make your project better.

• You should also really have a hook_help() with some basic info about the module.
• what is this project useful for? Why and when do people need it? What is the use case? Please improve your Project Page.: Extend this little bit more, better to go through Tips for a great project page again.
• Please also improve the README Template. Could be expanded more. See https://www.drupal.org/node/161085
• user_activity_log_block_content(): Don't use theme() directly, use render arrays. The main reason why the render api exists is it allows for altering by other modules before any html is produced. Also it provides one consistent system to produce any output. See https://www.drupal.org/node/930760

Since this was RTBC already and no major blocker jump out to me so...

Thanks for your contribution, chander bhushan!

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.