[D7] First Time Login

You might want to change the git-command. You can do this by unchecking the checkbox next to "Maintainer", on the Version Control-tab of your module-page.

You might also want to check out the automated project review on http://pareview.sh/pareview/httpgitdrupalorgsandboxneerajsingh2477075git

Hi neerajsingh,

Nice of you to update the module so quickly. However, the automated review shows that you haven't set the default branch of the module. No idea if this issue could be solved over time, but you might want to check the documentation on how to do so if it does not.

Marked this application to 'Needs review' for the time being.

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.

Updating project description as well for better understanding of Git command. :)

Hi neerajsingh,

See your code i see that you use

header("Location: $path");

To perform a redirect and also you are using exit.
You would to use drupal_goto to get the same result in a drupal way.

Also your code seems only 90lines of code when the min suggested for a contrib module is 120 lines

Limited scope of this project:
@neerajsingh, I have reviewed this project and found the limited scope. The functionality you are doing by this module can be achieved by rules and flag module easily. we should avoid writing code in drupal if something can be used to achieve the same type of functionality.

"Collaboration rather than competition" - https://www.drupal.org/contribute/development.
You can Try (https://www.drupal.org/node/683696) the ways to achieve the functionality you are developing.

Automated Review

Looks good!

Manual Review

Individual user account

Follows the guidelines for individual user accounts.

No duplication

Does not cause module duplication and/or fragmentation. Obviously most of this can be accomplished with a combination of other modules, but this module offers more specific functionality.

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

Follows the guidelines for in-project documentation and/or the README Template.

Code long/complex enough for review

Follows the guidelines for project length and complexity.

Secure code

Meets the security requirements.

Coding style & Drupal API usage

No issues found on initial inspection. Database calls use the abstraction layer and appear to be safe. Coding style appears to follow Drupal's standards and recommendations.

If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

This review uses the Project Application Review Template.

I'm wondering... DateObject class?

Re #12.
See line 147.

Sorry the number is wrong.

  $updated_date = new DateObject($details->updated_date);
  $current_time = new DateObject(REQUEST_TIME);

I'm referring to the snippet above. See your hook_user_login implementation.

... thus, making this Needs Work.
Sorry I tried to test the module but I don't have this class. Note that you can use DateTime class from PHP, that comes in all versions after 5.2. Drupal core requires 5.2 anyway, so you can use without requiring a PHP version explicitly in your module.

Thanks.
There's a typo in .module:151 ("aoount" instead of "account"). Not a blocker though.

Hi neerajsingh,

On first_time_login.module you don't need of $base_url to perform a redirect, you can do:

$path = "user/" . $account->uid . "/edit";

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

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.

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 / No: Does not follow] the licensing requirements.

3rd party assets/code

[Yes: Follows ] the guidelines for 3rd party assets/code.

README.txt/README.md

[Yes: Follows] the guidelines for in-project documentation and/or 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

[List of identified issues in no particular order. Use (*) and (+) to indicate an issue importance. Replace the text below by the issues themselves:

1. (*)When I clone the module then it was showing module name as "2477075" rather than "first_time_login"
2. (*)global $conf;line no 78, why the need of global variable $conf?
3. (*)global $base_url;line no 78, remove the global variable $base_url.
4. (*) $path = $base_url . "/user/" . $account->uid . "/edit";line no 104, used url() function as follow
   $path = url("user/" . $accountr->uid . "/edit", array('absolute' => true));
5. (*) if ($days_diff > $conf['first_time_login_config_days']) line no 98, used as follow
   if ($days_diff > variable_get('first_time_login_config_days')
6. (*)'@site_name' => $conf['site_name'], line no 113, you can used '@site_name' =>variable_get('site_name'); to get value of "site_name", follow same for all $conf variable.
7. (*) 'title' => 'First Time Login' line no 150,used t function as follow 'title' => t('First Time Login')
8. (*) Always create admin configuration page changes in new file such as first_time_login.admin.inc file so that it will be easy to handle
9. (*)first_time_login_form() line no 163, should be like this first_time_login_form($form, &$form_state)
10. (*)'#default_value' => $conf['first_time_login_config_days'], line no 161, you can used '#default_value' => variable_get('first_time_login_config_days'); to get value of "first_time_login_config_days"

$path = url("user/" . $accountr->uid . "/edit", array('absolute' => true));

neerajsingh,

While this functionality is easily replicable by something like Rules, I think your simplified implementation will help some people, and I don't think it warrants a limited scope blocker.

Code Review

The first thing I noticed is that you broke up your t() strings to adhere to the 80 character line limit in your hook_help(). Though I'm sure this is something Coder/pareview.sh had you address, most of the major contrib modules I've seen do not do this.

I also noticed you were using $account->is_new !=/== 1 instead of TRUE in your user_update and user_insert hooks. Obviously not a blocker, but I think the use of TRUE/FALSE where applicable would make your code slightly more readable.

This is also more of a stylistic change, but in the hook user login, you could simplify this excerpt to improve readability:

  if ($days_diff > $conf['first_time_login_config_days']) {
    $needsupdate = TRUE;
  }
  else {
    $needsupdate = FALSE;
  }

To the following:
$needsupdate = ($days_diff > $conf['first_time_login_config_days']);

I noticed you have double stacked comment blocks to explain the hook you're about to write. In most cases, it's more practical (and common) to do the following instead:

/**
 * Implements hook_somehook().
 * 
 * Explanation for hook.
 */

Lastly, when you're setting $account_created and $last_updated_date, I would prefer to see you use format_date() instead, so the date format isn't set in code, as this commonly differs by region/language. In future extensions I think the message text should also be configurable from your admin page, but that isn't necessarily prohibitive for the application process.

Manual Review:

The module looks simple. Although functionality seems to be serving the motive of module.

My recommendations for the new releases are as follows.
1. Improve UI in new releases, try to show the info in popup. It will improve user experience as well.
2. All messages should be made configurable.

Regards
Nitin Kumar

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:

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