First login

It's an interesting idea, but I'm seeing a few problems in the implementation.

- always wrap your ifs and elses in {}, even single lines.

- run through Coder module for formatting.

- when do first_login_set_data / first_login_get_data get called?

- user_save() takes two parameters IIRC.

- $user = user_load($uid);
I can't tell as I don't see where you're calling this, but if this is switching users, it's insecure.

- * Function first_login_get_data()
That does not count as documentation! ;)

    form_set_value($form['page_vis_settings']['pages'], "<?php\n  global \$user; \n return (first_login_get_data(\$user->uid, 'status') || \$user->uid == 1);\n?" . ">", $form_state);

Took me a while to figure this out... you're setting some PHP code into the visibility field...???
This does really not look good to me. What if the admin user already put something there? This is really hacky I'm afraid.

Overall, I think this would be a very handy feature for a lot of sites, but I think it would be better done a by providing a reaction plugin for Context module. A clean method of turning blocks on and off would be already provided for you, and you'd just need to let the API know the current user's status.

> I don't know how you meant it to be used as a reaction plugin, but now it's a condition plugin.

Nice job!

> How could it be implemented as a reaction?

I meant condition... sorry, was being dense :/

Don't get too anxious! Looks like the queue is way backed up, but best to ensure quality rather than a speedy process. My project has been in the queue for about a month now. I've learned a lot watching and reviewing other projects though.

Agreed. Good job!

Sorry for the delay...

This seems to be coming along very nicely.

Something that may require work has come up though...

I was having a poke around in your Views handlers, and noticed that they are fake fields, in the sense that they don't add anything to the view's query, but do a first_login_get_data() which in turn does a user_load(). This isn't the best way to do things. A better way would be for the handler to add the 'data' DB field to the query, and then deal with unpacking its data (here it *would* be sensible to use a helper function in the module).

This got me thinking... would storing the login count in a table be better? Which got me wondering how come nobody had done this already. Well, it turns out they have: http://drupal.org/project/user_stats.

So it seemed to me that it would be better to depend on a single module for handling login counts and all that.

However... looking at that module right now, I'm going 'WTF' at its data structure defined in its hook_schema. See for yourself: http://drupalcode.org/project/user_stats.git/blob/refs/heads/6.x-1.x:/us...

A blobby table like this is just crazy, especially when every single property that the module stores is one-to-one with a user.

So my recommendation with regard to data storage is keep doing what you're doing, and consider depending on user_stats in the future, when/if its data storage is cleaned up.

On to some simpler matters...

- first_login.context.inc

I don't think you need an inc file for just two API functions, it's the same as putting the views_api hook in the main module code. Do remember to give them docblock headers though!

- check blank lines. The end of every file should have an empty newline; there should be no blank line between docblock and function declaration.

- as previously mentioned, I think your Views handlers need to be changed.

  $form['first_login'] = array(
    '#type' => 'fieldset',
    '#collapsible' => TRUE,
  );

Don't put a fieldset around the entire form.

Closing due to inactivity, feel free to re-open if this was a mistake.

I am reviewing the module description for comparisons to other modules. Would I select the module for evaluation after reading the description?

The legal module lets you push the terms and conditions in front of other users at login. The condition is reset when the T&C changes. Will your "first time" condition allow for resetting and treating all people as first time if there is a major change to the site?

Could first time trigger a rule or add a role at the start of the first time period then trigger a rule or delete the role at the end? Hook first time start? Hook first time end?

Like you, I searched for something similar. In my case I wanted last-few-weeks-before-subscription-expired and first-few-weeks-after-subscription-starts. I wrote something that connects to everything else by adding a last-few role then a first-few role. A number of other modules and CSS can occur based on the roles. Ubercart roles does something similar for subscriptions. Based on your description of your module, I cannot see how it will connect to other modules, control the appearance of blocks, and CSS.

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

• Drupal Code Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.

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. Get a review bonus and we will come back to your application sooner.

manual review:

• you can ignore the method names warnings from the automated review.
• "if (module_exists('context')) {": no need for that check in the module file. If the context module is there, then the hooks will be called. If not then the function definitions also don't hurt.
• first_login_get_data(): the last else{} statement in that function is useless, remove it.

But that are just minor issues, otherwise I think this is RTBC. Removing review bonus tag, if there are still changes required you can add it again if you have done another 3 reviews of other projects.

Thanks for your contribution! Welcome to the community of project contributors on drupal.org.

I've granted you the git vetted user role which will let you promote this to a full project and also create new projects as either sandbox or "full" projects depending on which you feel is best.

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.

As you continue to work on your module, keep in mind: Commit messages - providing history and credit and Release naming conventions.