[D7] WebEngage Customer Engagement Suite

Hello, you need provide more information.
Read the Apply for permission to create full projects page on how to correctly set up a module review issue. And include the drupal version in the title of this issue (see step 5).

[general observation]
This module does well to follow the Drupal code standards.
I like the WebEngage app and this module integrates it well. The configuration page is pretty slick.

[manual review]
webengage.module:24 Use hook_permission in Drupal 7 instead of hook_perm.

webengage.info:5 Add the configuration path to the .info file. This will place a "Configure" link next to your module listing on the module overview page.

configure = admin/webengage/action/main

Note: I got the git repository link from the project's page but it nonetheless needs to be included in this issue's description.

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.

Hello!!

My manual review:

.module

In the hook_menu() I think that is more clear not use constants in the $item keys and put the value... the constants are defined in another file...

It's more clear write the hooks in .module instead in a lot of files and include all them.

  $items[PATH_CALLBACK] = array(
    'title' => 'WebEngage Settings',
    'description' => 'This is WebEngage Settings page',
    'page callback' => 'webengageDoActionCallback',
    'access callback' => 'user_access',
    'access arguments' => array('admin_only'),
    'type' => MENU_CALLBACK,
  );

The webengageDoActionCallback is in webengage_callback.php you can delete the line 18:

module_load_include("php", "webengage", "webengage_callback");

and change the $item[PATH_CALLBACK] to

  $items['admin/webengage/action/callback'] = array(
    'title' => 'WebEngage Settings',
    'description' => 'This is WebEngage Settings page',
    'page callback' => 'webengageDoActionCallback',
    'access callback' => 'user_access',
    'access arguments' => array('admin_only'),
    'file' => 'webengage_callback.php',
    'type' => MENU_CALLBACK,
  );

In webengage_callback.php you have webengage_preprocess_webengage_callback() but no code. You can remove it. not? or move it to .module

The .tpl files must be the views... You can pass all the arguments with theme() and hook_theme instead create and modify variables in the tpl files. See webengage.php.tpl. Please if I don't explain me, tell me.

In webengage.php.tpl you call the function webengage_process_webengage_options() You must include this file in the "logic" part and not in the template.

I think that you can understand me better if you see more about templates in drupal.

Sorry if you and another people think that I wrong but is confused the mix and have a lot of files with 5-10 codelines...

Your forms

you use .tpl to create the forms, you must use the drupal form api instead.

Summary

* Use the drupal form api to create the forms that configure, create, edit or delete content (like configuration, etc). See https://api.drupal.org/api/drupal/developer%21topics%21forms_api_referen... and https://drupal.org/node/1043838
* The template files can recieve an array so the variables or operations that you need to do can be sended to the template.
* In the hook_menu you can use the key file instead use module_load_include
* I'm tried to explained me. If you don't understand me please tell me.

good luck!

Hello.

I have to correct one of the points of my review. Sorry. In the review process of a module of mine told me that instead utilizase module_load_include required, so I adopted that point as a correction to make.

My module has passed the review process, but the person who enabled the publication of the project (cweagans) commented as follows on the subject:

For this bit:

 389 /**
 390  * include the block functions file
 391  */
 392 require_once realpath(__DIR__) . '/pay_with_a_tweet.blocks.inc';
 393 
 394 /**
 395  * include the tokens functions file
 396  */
 397 require_once realpath(__DIR__) . '/pay_with_a_tweet.tokens.inc';

1) To load module include files, you should use module_load_include; and
2) If you're including this unconditionally, there's no reason to split them out into another file. It is, in fact, a bit slower, since you're adding a couple of disk reads/file open operations into the mix. You could probably just copy the contents of those files into your .module file and you'd be fine.

I understand that, in this case, it is better to follow the recommendation of a person who can enable publishing projects.

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

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

Please do not RTBC your own issues, see https://drupal.org/node/532400

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 :-)

Hello

My manual review.

.module

lines 14s:

  module_load_include("php", "webengage", "webengage_constants");
  module_load_include("php", "webengage", "helper");
  module_load_include("php", "webengage", "process_options");
  module_load_include("php", "webengage", "webengage_main");
  module_load_include("php", "webengage", "webengage_callback");
  module_load_include("php", "webengage", "webengage_resize");

If you're including this unconditionally, there's no reason to split them out into another file. It is, in fact, a bit slower, since you're adding a couple of disk reads/file open operations into the mix. You could probably just copy the contents of those files into your .module file and you'd be fine.

webengage_main.php

 module_load_include("php", "webengage", "webengage_constants");

this line is not necessary. In .module the webengage_constants is already include.

webengage_main.tpl.php

  global $user, $base_root;
  $email = variable_get('site_mail', ini_get('sendmail_from'));
  $user_full_name = $user->name;

  $main_url = $base_root . base_path() . '?q=' . PATH_MAIN . '';
  $next_url = $base_root . base_path() . '?q=' . PATH_CALLBACK . '&noheader=true';
  $resize_url = $base_root . base_path() . '?q=' . PATH_RESIZE . '&noheader=true';
  $activation_url = $main_url . "&weAction=activate";

  if(isset($_SERVER['HTTP_HOST'])):
    $domain_name = $_SERVER['HTTP_HOST'];
  else:
    $domain_name = $_SERVER['SERVER_NAME'];
  endif;

  $webengage_host_name = "webengage.com";
  $webengage_host = 'http://' . $webengage_host_name;
  $secure_webengage_host = 'https://' . $webengage_host_name;
  $resend_email_url = $webengage_host . "/thirdparty/signup.html?action=resendVerificationEmail&licenseCode=" . urlencode($m_license_code_old) . "&next=" . urlencode($next_url) .
        "&activationUrl=" . urlencode($activation_url) . "&channel=drupal";

  webengage_process_webengage_options();

You can send all this values to your template using the hook_theme. The code at webengage_process_webengage_options() must be used with API Form and the hooks hook_form_submit and hook_form_validate.

The forms must be written with API form...

Lastly, regarding using forms API to create forms, again I'm sorry we can't use that since we give our customers "a consistent admin UI across different platforms, regardless of the inherent forms each platform supports". You might wanna check our modules on Magento, Joomla, Wordpress and a few other platforms whose work is in progress

I'm sorry. In Drupal you must build your forms using de form API. With API form you can get the same output that if you write the HTML code. I do not find a satisfactory answer that is done on other platforms as well.

All files:

Use the t() function to build in the text of your pages, this way your module can be translated.

For me, the use of HTML to write forms instead the API Form is a blocker. If you don't want use it, you can see to a bonus review (http://drupal.org/node/1975228) and if klausi say "ok"

PAReview: Individual user account

It seems you are using a non-individual account.
All user accounts are for individuals. Accounts created for more than one user or those using anonymous mail services will be blocked when discovered (see Get a Drupal.org account).

Can you confirm that the webengage account is a single user? Please change your account information and enter your realname.

If you prefer, we can also promote this sandbox to a full project without granting you "git vetted user" status.

----
Top Shelf Modules - Crafted, Curated, Contributed.

I agree with @rolando.caldas, the include files for constants are very confusing. Why would you need to keep the admin urls the same for Drupal and other CMSs? Drupal site builders expect to see configurations for modules in admin -> config. Line 49 in the .install file has the main link hardcoded too, "admin/webengage/action/main".

Two issues I see -
1 - Instead of creating a menu block through menu_save, you should create a new block using hook_block_info and hook_block_view. You can return a call to theme('links') for the block content which takes a special array of menu links, and then prints out a drupal menu with active classes and links. This is much better than creating a user custom menu. A drupal admin can still change the links and weight of the menu settings if they wish, the UI overrides module settings.

2 - You're calling drupal_get_form directly in the .tpl.php file for webengage_main.tpl.php. This should be called in your preprocess function and saved into a variable called $variables['form'], which you then just use drupal_render_children($form), and drupal_render($form['specific_element']) in the actual template file. I don't see a reason why lines 12-31 in that file shouldn't go in the preprocess function as well.

Hi,

Please find below comments:

1) Use db_query_range() instead of the SQL LIMIT clause
$num_existing_rows = db_query("SELECT * FROM {webengage_settings} LIMIT 1")->rowCount();
in webengage.install file.

2) Please see https://drupal.org/node/2054613#comment-7729101

Thanks

Adding git link

@webengage - yes, the account must be for an individual, not a company. You can change it to something like webengage_nitinbansal if you want to keep the company name in there. Also, provide your real name for the first/last name portion of the profile.

The process of getting final approval can take several months, please be patient. We know you are requesting full project access. Please keep in mind that all reviewers are volunteers and their ability to spend time here varies.

Full project review below:
Great project page!
"WebEngage Customer Engagement Suite" is a much better title for your module than the current "[D7] WebEngage Feedback, Survey and Notification" - this is only my opinion of course.

License

Please remove the licensing text in all files. All code hosted on drupal.org must be licensed GPLv2 or later, and drupal's packaging script will automatically add the appropriate information.

webengage.install

• on uninstall, you don't need to remove the table yourself, drupal will take care of that for you.
• why do you need to create rows with empty values? Surely when you later read from the database you will validate them anyway.
• That is just not the way we create menu items in Drupal - use hook_menu() to define the path and callback you want. You don't need to use webengage_constants here then either. asherry also mentioned this in #19, in his first point.

webengage.module

• instead of loading all the include files at the top, which will be part of every Drupal page request, just load them where you need them. Also, when using hook_menu, you can define a file parameter that will automatically load the include file for you.
• webengageDoActionMain, webengageDoActionCallback, webengageDoActionResize, webengageGetLicenseCode(), weAction, etc, we do not use camelCase in Drupal, please switch to underscores. see https://drupal.org/coding-standards#naming.
• you don't need to specify 'access callback' => 'user_access', on each menu item, that's the default access callback.
• In webengage_form_save_license_code_validate(), you can remove this function, instead just set the #required property to TRUE when you're defining the form element.
• In webengage_form_save_license_code_submit() you can use drupal_set_message() to let the user know what happened. Also, avoid drupal_goto() whenever possible, instead set the $form_state['redirect'].

webengage_callback.tpl.php
This looks like it creates a form and then submits it right away via javascript. Could you use curl to POST directly instead?

Remove webengage_feedback__survey_and_notification.info.

In webengage_preprocess_webengage_main(), we don't use the alternate php syntax if(isset($_SERVER['HTTP_HOST'])): ... endif unless you are in a tpl file.

I'm not sure what you're trying to accomplish with the separate callback/main/resize includes + template files - can you explain more about what you're trying to accomplish here? They just look like fairly normal pages, and Drupal already has methods for sending the user a message, creating forms, POSTing information, calling javascript programatically, etc. It seems like all of these should be refactored into simpler callbacks. If you really want to create your own menu during install, I guess that's ok and I won't complain about that.

Setting to needs work for the licensing issue, account issue, and lack of Drupal API usage.

----
Top Shelf Modules - Crafted, Curated, Contributed.

Looks a lot better, thanks! I'm still not sure about all the callbacks and extra template files, they look really inefficient.

----
Top Shelf Modules - Crafted, Curated, Contributed.

added project link

It's been a month without any problems reported, so I'm promoting this myself as per https://drupal.org/node/1125818.

Thanks for your contribution, webengage_nitin!

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:

• 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 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 - Crafted, Curated, Contributed.

Thanks for the positive feedback!