Submitting my first d.o module for review!

Module name:
User Auth.log

Detailed Description:
Allows for logging user actions on sensitive/critical administrative pages. Logging is limited to specific users and/or user roles (configurable). Logging only occurs on specific administrative pages (configurable). Features include:

  • Ability to log by user or users
  • Ability to log by role or roles
  • Ability to limit logging to specific paths
  • List view of all logged entries with pagination
  • Notifications emails sent upon specified user login
  • Notification emails sent when specified user disables the User Auth.log module
  • Presently the module will log user ID, path, and date/time

Intended core version:
7.x

Link to sandbox:
https://drupal.org/sandbox/kirschbaum/2170431

git clone --branch 7.x-1.x http://git.drupal.org/sandbox/Kirschbaum/2170431.git user_auth_log

Reviews of other projects:
https://drupal.org/comment/8394853#comment-8394853
https://drupal.org/comment/8396927#comment-8396927
https://drupal.org/comment/8398619#comment-8398619

Much thanks! Excited to be contributing to the community.

CommentFileSizeAuthor
#14 warnings.png12.63 KBnitesh sethia

Comments

Kirschbaum’s picture

Kirschbaum commented
Issue summary: View changes
sanguis’s picture

sanguis commented
Title: User Auth.log Module » [d7] User Auth.log Module
Assigned: Unassigned » sanguis

Assigning to my self for latter

adam_thomason’s picture

adam_thomason commented

Hi Kirschbaum,

I have taken the liberty of reviewing your code. For starters, great module. I only have one small recommendation, which is:

line 23 user_authlog.module:
require_once ( dirname(__FILE__) . '/user_authlog_email.inc');

require_once is a statement, rather than a function, and so parentheses aren't required.

Other than that, the module works great. Nice work.

Cheers,
Adam

Kirschbaum’s picture

Kirschbaum commented
Status: Active » Needs review
Kirschbaum’s picture

Kirschbaum commented

Hi Adam,

Thanks! I've made the update. Really appreciate you taking the time to review the module!

PA robot’s picture

PA robot commented

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.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Issue summary: View changes
PA robot’s picture

PA robot commented
Status: Needs review » Needs work

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

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

Kirschbaum’s picture

Kirschbaum commented
Status: Needs work » Needs review

Made some adjustments based on automated review tools results. Module is ready for review. Thanks!

Kirschbaum’s picture

Kirschbaum commented

As enzipher mentioned in his early review of the module, setting default variables in the .install file should not be required. At his recommendation I've updated the module accordingly.

tomasribes’s picture

tomasribes commented

Hi Kirschbaum,

I have been reviewing and testing your module and it works great! It can be very usefull in many contexts. Nice work!

In the future it will be nice to have more data saved in the log table. For example I tried to uninstall a system module and the log don't save witch one. It will be nice to have full data and availability of make queries using filters.

Cheers,
Tomas

Kirschbaum’s picture

Kirschbaum commented

Thanks for the review Tomas! Good idea for a feature request. I will add that to the list.

nitesh sethia’s picture

nitesh sethia commented
StatusFileSize
new issue.png15.47 KB

@Kirschbaum

I have been testing your module and found 2 warnings.
warnings

nitesh sethia’s picture

nitesh sethia commented
StatusFileSize
new warnings.png12.63 KB

Sorry I uploaded a wrong file.
warnings

Kirschbaum’s picture

Kirschbaum commented

Hi Nitesh,

Thanks! I'm aware of this and don't think it should be a show stopper for module approval. That said, if you disagree please provide a little more context and I'll be happy to address the issue.

Kirschbaum’s picture

Kirschbaum commented
Assigned: sanguis » Unassigned
Issue summary: View changes
Issue tags: +PAreview: review bonus
Kirschbaum’s picture

Kirschbaum commented

Hi Nitesh,

I wanted to follow up on my previous comment about only passing string literals to the t() function. I think this might be a use-case where this can be overlooked. The reason being that the variable I am passing in will be one of two possible strings. The benefit to the approach I'm using is that it prevents me from repeating code. For example, I'm currently doing this:

  switch ($key) {
    case "user_login":
      $subject = 'User @name logged in to @site-name';
      $body    = 'The user @name logged in to the website "@site-name" on @date';
      break;

    case "module_disable":
      $subject = 'User @name at @site-name disabled User Authlog';
      $body    = 'The user @name from the website "@site-name" disabled the User Authlog module on @date';
      break;
  }

  ...

  $message['subject'] = t($subject, array(
    '@name' => $user->name,
    '@site-name' => $site_name,
  ), $options);

  $message['body'][]  = t($body, array(
    '@name' => $user->name,
    '@site-name' => $site_name,
    '@date' => date("F j, Y, g:i a"),
  ), $options);

Whereas to fix the warning I believe I'd do something like this:

  switch ($key) {
    case "user_login":
      $message['subject'] = t('User @name logged in to @site-name', array(
        '@name' => $user->name,
        '@site-name' => $site_name,
      ), $options));
      $message['body'] = t('The user @name logged in to the website "@site-name" on @date', array(
        '@name' => $user->name,
        '@site-name' => $site_name,
      ), $options));
      break;

    case "module_disable":
      $message['subject'] = t('User @name at @site-name disabled User Authlog', array(
        '@name' => $user->name,
        '@site-name' => $site_name,
      ), $options));
      $message['body'] = t('The user @name from the website "@site-name" disabled the User Authlog module on @date', array(
        '@name' => $user->name,
        '@site-name' => $site_name,
        '@date' => date("F j, Y, g:i a"),
      ), $options));
      break;
  }

I think the concern with passing a variable into a t() function is that there might be countless variations, which is an obvious problem for anyone trying to provide a translation for all possible outcomes. I'm not an expert on the t() function, but I don't think that would be the case here because the variable I am passing in would be one of two possible strings. That said, I'd be happy to hear from you or someone else who has experience in this area.

Thanks again.

Kirschbaum’s picture

Kirschbaum commented

Thanks for the reviews all! Assuming no further feedback, could one of you please change the status to 'RTBC' so a git administrator can give final validation?

Kirschbaum’s picture

Kirschbaum commented
Issue summary: View changes
Kirschbaum’s picture

Kirschbaum commented
Issue summary: View changes
klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Assigned: Unassigned » patrickd
Status: Needs review » Reviewed & tested by the community
Issue tags: -PAreview: review bonus

Review of the 7.x-1.x branch:

  • Coder Sniffer has found some issues with your code (please check the Drupal coding standards).
    
FILE: /home/klausi/pareview_temp/user_authlog_email.inc
--------------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 2 WARNING(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
 55 | WARNING | Only string literals should be passed to t() where possible
 60 | WARNING | Only string literals should be passed to t() where possible
--------------------------------------------------------------------------------

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. The 2 automated warnings above are right: you should run the contents of $subject and $body through t() when you assigning the variables, otherwise the strings cannot be found by translation extraction tools.
  2. user_authlog_entry_insert(): "When handling data, the golden rule is to store exactly what the user typed. When a user edits a post they created earlier, the form should contain the same things as it did when they first submitted it. This means that conversions are performed when content is output, not when saved to the database." from https://drupal.org/node/28984 , so the check_plain() is wrong here and you will also not need user_authlog_clean_array().
  3. user_authlog_page_alter(): you are not actually altering the page, so it looks like you want hook_page_build() instead?
  4. user_authlog_user_login(): the check_plain() on the description is useless here, since it does not contain user provided text? Please don't fix issue from automated tools blindly, there might be false positives. Also make sure to read handle text in a secure fashion again: https://drupal.org/node/28984
  5. user_authlog_mail_alter(): why are that headers necessary? Please add a comment.
  6. user_authlog_form(): the user query will not scale on larger sites with thousands of users. It will also kill your browser performance when you get such a large selectbox. Use a user name autocomplete field instead, like the node edit form for authors for example or something similar.
  7. "'#title' => 'Track by role',": all user facing text must run through t() for translation, please check all your strings (all #title for example).

Although you should definitely fix those issues they are not critical application blockers, so I guess this is RTBC. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

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

patrickd’s picture

patrickd commented
Status: Reviewed & tested by the community » Fixed

sorry for the delay

  1. The .gitignore file contents don't make much sense in the module folder, what use does it have in your repository?
  2. Your installation message tells the user that he has to configure the module first; why does the message not link to the configuration page?

That's nothing critical though..

Thanks for your contribution!

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:

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.

Kirschbaum’s picture

Kirschbaum commented

Hi Klausi,

Really appreciate the review. I've addressed the issues you pointed out:

  1. Fixed.
  2. Fixed.
  3. Fixed.
  4. Fixed.
  5. You are right. They aren't needed. Fixed.
  6. Updated with username autocomplete field.
  7. Reviewed and made several corrections.

Thanks again for taking the time to review the module!

Kirschbaum’s picture

Kirschbaum commented

Hi patrickd,

Regarding the two issues you point out:

  1. Removed.
  2. Added.

Thanks for the final review!

Kirschbaum’s picture

Kirschbaum commented
Status: Fixed » Needs review

Hi patrickd,

It seems that I still don't have permissions to promote this project or create new full projects. Can you confirm the necessary permissions have been granted?

Thanks!

patrickd’s picture

patrickd commented

whoops, sorry! now you got it

Kirschbaum’s picture

Kirschbaum commented
Status: Needs review » Fixed

Much thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.