The Addictive Points module allows Drupal site owners to easily and quickly configure and install the needed Javascript code to use the Addictive Points platform.

Once the 'addictivepoints' folder is installed in the modules folder, the user can activate it through the module admin screen (admin/build/modules).

After activation, the user can navigate to the module administration page in order to enter a unique API key which identifies the site to the Addictive Points site. The key that is entered is validated to ensure it contains 40 characters. The user is also asked where the Addictive Points tab should appear on their site; the tab allows site visitors to sign up to the Addictive Points Loyalty scheme.

When the correct details have been submitted and the configuration data is saved, the module saves the two variables (to the variables table), in order to store the tab position and the API key.

The module uses hook_init to get a 'HEREDOC' (in order to improve readability and avoid issues with escaping characters) version of the javascript as a string. The javascript has placeholders written in for where the API Key and the Tab Position variables will be replaced with the submitted data. This Javascript is written into each page to create the Addictive Points tab.

Module is for Drupal version 6.x.

This module is similar to the Inject module in that it injects some Javascript but it differs in that it offers an easy to use form to enter the API key and the position for the tab to appear. It also validates that the key is the correct number of characters in length giving useful feedback if the copy / paste process didn't work well.

Project page : http://drupal.org/sandbox/TeeHays/1823016
Git : TeeHays@git.drupal.org:sandbox/TeeHays/1823016.git

Comments

ymakux’s picture

ymakux commented
Status: Needs work » Needs review

Hello
first you have to create another branch. You have "version_6", but it is not valid name

see Moving from a master to a major version branch

Remove addictivepoints-6.x-1.0.tar.gz and addictivepoints-6.x-1.0.zip

function apointsjscode() - wrong name, it should be something like MODULE_apointsjscode, or, better yet _MODULE_apointsjscode

apoints_form_validate($element, &$form_state='') {

The proper syntax is

MYMODULE_formname_validate($element, &$form_state);

Also please format your code then check it here

ymakux’s picture

ymakux commented
Status: Needs review » Needs work
TeeHays’s picture

TeeHays commented

Thanks for the initial review, have addressed all mentioned points and would like another review please.

Project page : http://drupal.org/sandbox/TeeHays/1823016
Git : TeeHays@git.drupal.org:sandbox/TeeHays/1823016.git

TeeHays’s picture

TeeHays commented
Priority: Normal » Major

Another point to note: This module is incredibly small and comprises just one file and five small functions which allow for the addition of some javascript code which allows the addictivepoints platform to work on drupal powered websites. It would be a very quick file to review.

h3rj4n’s picture

h3rj4n commented
Status: Needs review » Needs work

You still haven't fixed the proper syntax for the hook_form_validate (line 163). See comment #1 for explanation. (See search_form_validation as example)

You use a variable in the t() function on line 166. Problem with this is that every variable needs a translation. See the t() api for more information.
A small summary of the solution, replace the variable with @key_length and put the variable in the array.

In your hook_menu you still got text that doesn't goes trough the t() function. Please be shure that all user facing text goes trough t() for translation (comment from klausi point 7).

TeeHays’s picture

TeeHays commented
Status: Needs work » Needs review

Many thanks for the reviews;

I have addressed all the issues as best as I can; though there are a couple of areas I am not completely confident about:-

hook_menu : I wrapped some of the hook_menu text in the t() function, but pareview is throwing errors at me:

FILE: ...w/sites/all/modules/pareview_temp/test_candidate/addictivepoints.module
--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
111 | ERROR | Do not use t() in hook_menu()

112 | ERROR | Do not use t() in hook_menu()

hook_form_validate :

It wasn't clear to me from the examples given, how the anatomy of the validate function name is generated.

I got MYMODULE = addictivepoints
I've guessed that _form_ is the central part.
And Validate is the end section.

However, it was easy for me to consider that the hook_form_ function name could easily be the central part of the hook_form function name, or perhaps the first index of the form array would be the central part?

If this is still incorrect, please could you just let me know what it should actually be called. Also, because I'm using element_validate, does this make a difference?

Thanks in advance.

h3rj4n’s picture

h3rj4n commented

I read up on the hook_menu() api and it seams I was wrong xD As explained in the following lines:

title": Required. The untranslated title of the menu item.
"description": The untranslated description of the menu item.

Title and description should not go trough t(). I thought the description needed to go trough t().

As defined in your hook_menu, your form id is 'addictivepoints_apikeyget_form':
'page arguments' => array('addictivepoints_apikeyget_form'),

That means that Drupal will automatically search for the functions addictivepoints_apikeyget_form_validate, and addictivepoints_apikeyget_form_submit for the default form flow.

So if you change the function name from 'addictivepoints_form_validate' to 'addictivepoints_apikeyget_form_validate' and loose line 139 it should work for both fields (unless you only want to validate the first field). If you change this you'll have to change the entire validate function.

Right now you only check the field you've with the #element_validate. See this example for right syntax (Form API). Basically you'll have to the following (line 163):

function addictivepoints_form_validate($element, &$form_state='') {

Change to:

function addictivepoints_form_validate($element, &$form_state, $form) {

Why do you have the javascript inside your module file instead of a seperate file? And why do you insert it inline? It's also possible to use varaibles on the places you're now replacing with a string and use Drupal javascript variables to set the vars.

I'm not shure if hook_init is the way to go to insert the javascript on all pages because it's not executed on cached pages (see the API). But it could be cached with all the pages because init was run on first view of the page.

h3rj4n’s picture

h3rj4n commented
Status: Needs review » Needs work

Forgot to set it to needs work.

TeeHays’s picture

TeeHays commented
Status: Needs work » Needs review

Thanks once again for the review:-

* Removed the t() function calls from hook menu.
* Re-written the hook_form_validate function as suggested, thanks for explaining.

I've attempted to re-write the way that the javascript is being called, but have run into problems the suggested format.

/**
 * Implements hook_init().
 *
 * Installs a piece of javascript on each page if the
 * apoints_key variable has been set.
 */
function addictivepoints_init() {
  $mode = 'parent';
  if (module_exists('overlay')) {
    $mode = overlay_get_mode();
  }

  if ($mode != 'child') {
    if (variable_get('apoints_key', TRUE)) {
      $var_pos = variable_get('apoints_pos', 'topright');
      $key = variable_get('apoints_key', '');
      if (strpos($var_pos, 'left') !== FALSE) {
        $x_pos = 'left';
      }
      else {
        $x_pos = 'right';
      }
      if (strpos($var_pos, 'bottom') !== FALSE) {
        $y_pos = 'bottom';
      }
      else {
        $y_pos = 'top';
      }      
      $addictivepoints_js_settings = array(
        'api_key' => $key,
        'tab_x' => $x_pos,
        'tab_y' => $y_pos,
      );
      $path = drupal_get_path('module', 'addictivepoints');
      drupal_add_js(array('addictivepoints' => $addictivepoints_js_settings), 'setting');
      drupal_add_js($path.'/addictivepoints.js', 'module');
    }
  }
}

The js file is :

var apoints_api_key = Drupal.settings.addictivepoints.api_key;
var tab_x = Drupal.settings.addictivepoints.tab_x;
var tab_y = Drupal.settings.addictivepoints.tab_y;

var _apapi = _apapi || {};
_apapi.settings = {
        key:"'" + apoints_api_key + "'",
        position: {x:"'" + tab_x + "'", y:"'" + tab_y + "'"}};

(function(){
  var apoints = document.createElement("script");
  apoints.type = "text/javascript"; apoints.async = true;
  apoints.src =
       ("https:" == document.location.protocol ? "https://" : "http://")
                 + "www.addictivepoints.com/api/js/addictive-points.js";
  var s = document.getElementsByTagName("script")[0];
  s.parentNode.insertBefore(apoints, s);})();

I get the following message:-

TypeError: Drupal.settings.addictivepoints is undefined
var apoints_api_key = Drupal.settings.addictivepoints.api_key;

Which looks very much like : this problem : http://drupal.org/node/1164570

I implemented the javascript inline because it's just a small snippet of code; and hook_init seemed to be the best for being attached to every page.

If there is a better hook to use, I'd be very grateful if you could let me know. Also, I did try to find a way of implementing js code so that it would be available on cached pages, but there is very little documentation on this that I could find.

Thanks in advance for any other tips you can give me, all best regards

TeeHays

jphelan’s picture

jphelan commented
Status: Needs review » Needs work

line 115 - You set the page access to access administration addictivepoints. You need to declare it with hook_perm else it will not be on the permissions page and only the uid 1 admin will be able to access the page.

line 137 - consider pulling the web address out of the t() function and create an actual link with the l() function.

line 143 - your strong tag is stripped out by the t() function. You need to pull it out and concatenate.
http://api.drupal.org/api/drupal/includes%21common.inc/function/t/6

As far as you js file is concerned. Implement your code as a behavior, this will insure the settings are loaded by the time your code runs. That should fix the issue.

Drupal.behaviors.addictivepoints = function (context) {

    // your code here

  };

A couple small things you can review here:
http://ventral.org/pareview/httpgitdrupalorgsandboxteehays1823016git

This is not a super complicated module, are you planing on a 7.x version? It should not be difficult to port over.

PA robot’s picture

PA robot commented
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.

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

TeeHays’s picture

TeeHays commented
Assigned: Unassigned » TeeHays
Status: Closed (won't fix) » Needs review

Thanks for the review jphelan.

I've addressed those points you've made as far as I can tell.

* I've changed the access from 'access administration addictivepoints' to just addictivepoints because it makes most sense.
* I've created a link using the function, would appreciate your feedback on how I've done it and if it could be done better.
* Also did what you suggested with concatenating the module name into a separate string.
* Also put the js script inside the Drupal.behaviours.addictivepoints declaration, I think this is correct.

Once this module passes muster I will certainly port it to version 7, but due to my lack of experience with Drupal I just want to focus on one thing at a time! :-)

Thanks once again for your review, very helpful!

All best regards

TeeHays.

pobster’s picture

pobster commented

Afraid your hook_perm is incorrect - https://api.drupal.org/api/drupal/developer!hooks!core.php/function/hook...

It should simply be;

function addictivepoints_perm() {
  return array('addictivepoints');
}

Although it should really be a bit more descriptive, e.g. 'access addictivepoints' - note if you do change it then ensure you update the access in your hook_menu as well.

Also, try to understand more about what a t() call is actually for. It's short for "translatable string" with the idea being in other languages when translated, the ordering of the words may be different to how they are in English. With this in mind, remember that's why t() has placeholder tokens, so the user input can be passed in rather than be in a specific place in the string.

'#title' => t('Select Position for the ') . $addictivepoints_name . t(' tab'),

Should be;

'#title' => t('Select Position for the @name tab', array('@name' => $addictivepoints_name)),

To see this in action go to http://translate.google.com/#auto/fr/Select%20Position%20for%20the%20%22... which shows the quoted text at the end of the sentence, not before the word 'tab'.

Thanks,

Pobster

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.

TeeHays’s picture

TeeHays commented

Thanks for the review pobster,

Again, very grateful for the specifics. I've addressed those issues you pointed out and have uploaded corrections. I've done as you suggested and made the access name more descriptive. I've also changed the title to use the t() function properly.

Please can you let me know if there are any further changes needed.

All best regards

TeeHays.

TeeHays’s picture

TeeHays commented
Priority: Major » Normal
sreynen’s picture

sreynen commented
Title: Addictive Points plug and play rewards platform. » [D6] Addictive Points plug and play rewards platform.

Adding Drupal version to title to make this a little easier for reviewers to find.

kscheirer’s picture

kscheirer
Primary language English
Location Vallejo
commented
Status: Needs review » Reviewed & tested by the community

In addictivepoints.module use single quotes for your strings, it's slightly faster and adheres to Drupal's standard. You only need double quotes if you have variables inside the string. See https://drupal.org/coding-standards#quotes.

In addictivepoints_init() you have variable_get('addictivepoints_key', TRUE) - shouldn't that default to FALSE instead? You only want to include your javascript when it's been specifically enabled.

I'm not sure why you need the $addictivepoints_name variable, since it's always the same string - perhaps a define() or variable_get() instead?

You still have a problem with the javascript - it's defined both in addictivepoints.js and _addictivepoints_jscode(). I don't see where addictivepoints.js is being used at all., so I think you can safely remove that.

No major issues found though, security looks ok.

----
Top Shelf Modules - Enterprise modules from the community for the community.

kscheirer’s picture

kscheirer
Primary language English
Location Vallejo
commented
Assigned: TeeHays » Unassigned
Status: Reviewed & tested by the community » Fixed

Well it's been almost a month and the code still looks decent enough - cleaning up your javascript calls would be nice.

Thanks for your contribution, TeeHays!

I updated your account to let you 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 get 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.

----
Top Shelf Modules - Enterprise modules from the community for the community.

Status: Fixed » Closed (fixed)

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

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented