Drupal.org
Issues

First time visit

Posted by johnnygamba on November 27, 2012 at 1:46pm
Project:Drupal.org Project applications
Component:module
Category:task
Priority:normal
Assigned:Unassigned
Status:closed (won't fix)

Issue Summary

This module sets a cookie the first time a user comes into the site and exposes a function called is_first_time_visit(); This is done via javascript to guarantee the code executes even if a proxy reverse is implemented.

Link to project: http://drupal.org/sandbox/johnnygamba/1851384
Link to git repository: git clone --recursive --branch master johnnygamba@git.drupal.org:sandbox/johnnygamba/1851384.git first_time_visit
Drupal 7

Thanks.

Login or register to post comments

Comments

#1

Posted by idflood on November 28, 2012 at 11:03am
Status:needs review» needs work

Using the following as a structure for my review http://drupal.org/node/894256 and http://groups.drupal.org/node/184389
Manual review:

README.txt
Please take a moment to make your README.txt follow the guidelines for in-project documentation.
project page
Maybe add a little example of how this module could be used.
Master Branch
It appears you are working in the "master" branch in git. You should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.
Major coding standards / best practice issues
An automated review of your project has found some issues with your code; As coding standards make sure projects are coded in a consistent style we please you to have a look at the report and try to fix them. Anyway, note that issues found are possibly false positives and fixing all issues is not a requirement for getting through the application process.

You can find the results of the automated report at http://ventral.org/pareview/httpgitdrupalorgsandboxjohnnygamba1851384git.

Code too short
This project is too short to approve you as git vetted user. We are currently discussing how much code we need, but everything with less than 120 lines of code or less than 5 functions cannot be seriously reviewed. However, we can promote this single project manually to a full project for you.

first_time_visit.js
On line 4, the class passed to the "once" function should be less generic I think. Maybe something like this:

$('body', context).once('first_time_visit_processed', function(){

first_time_visit.module
Maybe rename "is_first_time_visit" to "first_time_visit_is_first" or something like that.

#2

Posted by johnnygamba on November 28, 2012 at 2:46pm

hi thanks for the review, your help is much appreciated.

#3

Posted by johnnygamba on November 29, 2012 at 5:08pm

Hi @idflood, i did everything you pointed me, can you review it again please? thanks.

#4

Posted by johnnygamba on December 2, 2012 at 7:53pm
Status:needs work» needs review

review please

#5

Posted by klausi on December 2, 2012 at 9:10pm

We are currently quite busy with all the project applications and I can only review projects with a review bonus. Please help me reviewing and I'll take a look at your project right away :-)

#6

Posted by johnnygamba on December 3, 2012 at 12:03am

Ok i will give it a try, i will be letting you know, thanks.

#7

Posted by johnnygamba on December 3, 2012 at 8:34pm

I have already reviewed one, but it hasn't passed yet, i'll be informing you about my progress helping the community.
cheers

#8

Posted by monymirza on December 4, 2012 at 7:23am
Status:needs review» needs work

Hi,

need to clear some errors/warnings. see here: http://ventral.org/pareview/httpgitdrupalorgsandboxjohnnygamba1851384git

first_time_visit.module
(optional) can be add hook_help()

You still have good review at #1. plz complete your work following that.

#9

Posted by johnnygamba on December 4, 2012 at 5:04pm

hi, thanks for the review, i just fixed everything, cheers.

#10

Posted by lolandese on December 4, 2012 at 7:57pm
Status:needs work» needs review

i just fixed everything

Don't forget to change status then. :)

#11

Posted by johnnygamba on December 4, 2012 at 9:36pm

Sorry man change the status to what?, i don't know what to do next.
thanks for your time.

#12

Posted by netol on December 5, 2012 at 11:22pm

first_time_visit.module
You should use isset() inside first_time_visit_is_first().
You could implement the function as follows:

function first_time_visit_is_first() {
  return isset($_COOKIE['first_time_visit']) && $_COOKIE['first_time_visit'];
}

or better:
function first_time_visit_is_first() {
  return !isset($_COOKIE['first_time_visit']) || $_COOKIE['first_time_visit'];
}

first_time_visit.js
You have:

$('body', context).once('first_time_visit_processed', function(){
  if (!$.cookie('first_time_visit')) {
    $.cookie('first_time_visit', true, { expires: 5000 });
  }
  else {
    $.cookie('first_time_visit', false);
  }
});

Honestly, I don't see how this can work. If the cookie is not set or is false, you set it to true. If the cookie is true, you set it to false. But the next time, the cookie will be set to true again.
Maybe you want to do this: ??
$('body', context).once('first_time_visit_processed', function(){
  if ($.cookie('first_time_visit') === null) {
    $.cookie('first_time_visit', true, { expires: 5000 });
  }
  else {
    $.cookie('first_time_visit', false);
  }
});

Please also align all the javascript code properly.

README.txt
The file is short. Could be extended explaining how the module works and what it uses.

#13

Posted by netol on December 5, 2012 at 10:46pm
Status:needs review» needs work

#14

Posted by klausi on February 15, 2013 at 7:13pm
Status:needs work» closed (won't fix)

Closing due to lack of activity. Feel free to reopen if you are still working on this application.

If you reopen this please keep in mind that we are currently quite busy with all the project applications and I can only review projects with a review bonus. Please help me reviewing and I'll take a look at your project right away :-)

nobody click here