[D7] Scan-to-Login

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

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.

Change git clone link

Hi Zapper Limited,

Great idea for a module! Some pointer before you release:

1. scan_to_login.info:6 Should this stylesheet be included in every page load? If it is only necessary for styling the login form, you can use the #attached property.
2. scan_to_login.module:16 This code should preferably be in hook_init(). When this hook is called, the theme and all modules are already loaded in memory.
3. scan_to_login.module:197 This class should be in a seperate file and conditionally included using module_load_include() or as a seperate file in your .info file
4. scan_to_login.module:70 You can add your page callbacks for the admin interface to a seperate file, so they can be loaded when needed. Your .module file should be a small as reasonably possible to reduce memory load. You can read more on how to include files for page callbacks in hook_menu()
5. scan_to_login.module:128 Environment variables are typically placed in settings.php, so they can be different per environment and dont have to be hardcoded in your repository.
6. scan_to_login.module:171 Should these scripts be included in every page load? If it is only necessary for adding functionality to the login form, you can use the #attached property.
7. To prevent jQuery from being included twice, you should test for it first. Preferably, let the site developer use a generic solution like jQuery update to prevent jQuery version collisions.
8. scan_to_login.module:286 You can use the drupal version drupal_json_output() to return the response and set the content headers.
9. js/scan_to_login.zappertech.inc.js External libraries should be included using the Libraries API, so they can be reused by other modules
10. js/scan_to_login.bootstrap.inc.js:23 Instead of testing for the html id attribute, attach your html placeholders using hook_form_FORM_ID_alter and let your jQuery selectors use your own markup.

Hello,

In classes / scantologinhelper.inc some of the arrays does not follow the Drupal coding standard for arrays

scan_to_login_init() is quite heavy, and will probably cause quite a performance loss. Take a look at hook_form_alter() and my Authy Authy module for some inspiration on how to "intercept" logins.

  92   // Set the environment we're using.
  93   if (isset($conf['s2l_environment_url'])
  94     && !empty($conf['s2l_environment_url'])) {
  95 
  96     $s2l_api_url = $conf['s2l_environment_url'];
  97   }
  98   else {
  99     $s2l_api_url = 'https://zapapi.zapzap.mobi/zappertech';
 100   } 

can be replaced by

$s2l_api_url = variable_get('s2l_environment_url', 'https://zapapi.zapzap.mobi/zappertech');

Good luck!

• Your git clone link here is your personal git clone command, change that.
• Files in git shouldn't have execution rights!
• Don't split translatable string like in scan_to_login_help function.
• Fix pareview issues.
• Implement install file to remove variables on uninstall.

Hey Zapper Unlimited,

For an example how to include an external library in your module, have a look at one of this project:
- https://drupal.org/project/chosen: http://drupalcode.org/project/chosen.git/tree

If you wan't it to have different kind of legal licence than Drupal then you can't include it in your module.

Hi Jonathan

There are 2000+ errors in your code as per parview.sh. Please use pareview as a first level of check. Although these are minor indentation/spacing errors - so I am not blocking the review of your code. But, please make the changes to make your module adhere to Drupal coding standards.

Thanks
Raman

Hi,

Coding standard check as per the pareview displays the following, can you please look into it?

Attached a file for your reference.

Hey Zapper,
According to me, you did superb work, really appreciate that. The code seems to be nice and well maintained.
I have few suggestion:-
1. on line 70-- if ($action == 'authenticate') {
and also on line 77 --- elseif ($action == 'register') {
Here you are comparing it by one string, it has to be translatable. Please pass t() to strings.

2. Why don't you have used switch case rather than elseif?
Better to use switch case according to me.

But apart from this, everything looks fine.

You need to do the review bonus or the moderators won't take a look at your project.

I have followed this sandbox project reviewstring with keen interest. Moderators pls do the needful and lets see the module live at drupal.org. Am quite sure a lot more people are waiting for projects like this.

> description = A module that allows you to scan a QR code to register and log in on a drupal site

Needs a full stop. Also, users already know that this is a module and that it's for Drupal sites!

 * @file
 * Contains ScanToLoginHelper
 * A module that allows you to scan a QR code to register 
 * and log in on a drupal site.
 * Created by ZapGroup.
 *
 * @package scan_to_login.module

First sentence after @file should fit on a single line. Is @package a standard docblock code? I don't recognize it.

 * Implements hook_help().
 * 
 * Displays scan to login's help and module information

No need to document what hook implementations do, unless they do something out of the ordinary.

      $html .= '<p>' . t("Adds a QR code to the login window for registration and log-in") . '</p>';

Missing full stop.

/**
 * Adds the Scan to Login configuration item to the main configuration page.
 */
function scan_to_login_menu() {

Incorrect docs -- this is a hook implementation.

    'description' => 'Configuration for the Zapper Scan-to-Login module',

Should be a verb in the imperative. Look at other config menu items for examples to follow.

function scan_to_login_form_alter(&$form, $form_state, $form_id) {

  switch ($form_id) {
    case 'user_login_block':

Better to use hook_form_FORM_ID_alter() here.

        if ($action == 'authenticate') {
          if (!empty($_POST['username']) && !empty($_POST['password'])) {
            $username = filter_var($_POST['username'], FILTER_SANITIZE_STRING);
            $password = filter_var($_POST['password'], FILTER_SANITIZE_STRING);
            $scan_to_login_helper->authoriseUser($username, $password);
          }
        }

It looks a lot to me like you are trying to work with the form submission in a form builder. If that's the case, then it's a total misuse of FormAPI. What are you trying to do here? (Can't tell without any code comments!!)

 * @file
 * A module that allows you to scan a QR code to register 
 * and log in on a drupal site.

That's not true in the admin.inc file.

      '#default_value' => variable_get('scan_to_login_merchant_id', 0),

Are you sure you want to put a '0' in the field when the user first goes to configure? I'd find it awkward UX to have to remove that 0 before entering my ID.

For a full description of the module, visit the project page:
  http://drupal.org/project/<project_name_here>

Hm....

* jQuery 1.7.2 or later JavaScript library (get's included when 
  the module is enabled)

Spelling mistake. Also, loading a different jQuery version automatically is a bad idea. Better to add a dependency of jquery_update module.

Git clone has an empty scan_to_login folder it seems (though not sure how, as git doesn't track folders?! Maybe that's a glitch at my end.

> * Authorise a user programmatically through the drupal "api".

No need for scare quotes!

  public function authoriseUser($username, $password) {

Needs parameter documentation.

      $user_obj = user_load_by_name($username);

      $form_state = array();
      $form_state['uid'] = $user_obj->uid;

      user_login_submit(array(), $form_state);

You're calling the submit handler for another form with faked data!!! That's a total hack, and bound to cause problems. If you want to log in a user, there's an API function for that.

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

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