[D7] JS confirm Pop Up

Git clone command for the sandbox is missing in the issue summary, please add it.

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.

Manual Review

Individual user account

[Yes] the guidelines for individual user accounts.

No duplication

[Yes] module duplication and/or fragmentation.

Master Branch

[Yes branch 7.x-1.x ] the guidelines for master branch.

Licensing

[Yes ] the licensing requirements.

3rd party assets/code

[Yes] the guidelines for 3rd party assets/code.

README.txt/README.md

[Yes] the guidelines for in-project documentation and/or the README Template.

Code long/complex enough for review

[Yes] the guidelines for project length and complexity.

Secure code

[Yes: Meets the security requirements. / No: List of security issues identified.]

Coding style & Drupal API usage

1. (*) On line 14,21 : All variables defined by your module must be prefixed with your module's name to avoid name collisions with others.
2. (*) On line 44 in js_confirm_pop_up.js there should not be white space after an opening "(" .
3. Expected 1 newline at end of file

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

This review uses the Project Application Review Template.

Please add configuration link in the module listing page, otherwise we will not be able to navigate to the config page with out url.
eg: configure = admin/config/user-interface/jsconfirmpopup in your info file

Hi,


1 - I don't understand why in your settings form you have two fields with two variables set :
js_confirm_pop_up_confirm_form_id and js_confirm_pop_up_actual_form_id
As i read your main function in your module :

function js_confirm_pop_up_form_alter(&$form, &$form_state, $form_id) {
  $form_ids = variable_get('js_confirm_pop_up_actual_form_id');
  $form_ids_arr = explode(',', $form_ids);
  if (!empty($form_ids_arr)) {
    foreach ($form_ids_arr as $value) {
      if ($form_id == $value) {
        drupal_add_js(drupal_get_path('module', 'js_confirm_pop_up') . '/js_confirm_pop_up.js');
        drupal_add_js(array('js_confirm' => array('form_ids' => variable_get('js_confirm_pop_up_confirm_form_id'))), 'setting');
      }
    }
  }
}

i think the first variable is for the php and the second for the javascript - but it is unnesessary. You should have only one variable js_confirm_pop_up_confirm_form_id and in your form_alter function you should for instance add a special class to your form tag in order to make it detectable by your script :

function js_confirm_pop_up_form_alter(&$form, &$form_state, $form_id) {
  $form_ids = variable_get('js_confirm_pop_up_actual_form_id');
  $form_ids_arr = explode(',', $form_ids);
  if (!empty($form_ids_arr)) {
    foreach ($form_ids_arr as $value) {
      if ($form_id == $value) {
        drupal_add_js(drupal_get_path('module', 'js_confirm_pop_up') . '/js_confirm_pop_up.js');
        if (!isset($form['#attributes']['class'])) {
          $form['#attributes'] = array('class' => array('js-confirm-pop-up'));
        }
        else {
          $form['#attributes']['class'][] = 'js-confirm-pop-up';
        }
      }
    }
  }
}

Then in your javascript you just need to detect any form with the ''js-confirm-pop-up' class.


2 - I think you should check if the form really changed. If i edit a field in the form and then cancel my changes by typing exactly the same i still have the popup displayed.
Maybe you should store in the attach function all the inputs state and then compare it to the form state when the user is leaving.

You have also an encoding problem in your README.txt file

EDIT: removed long pareview.sh dump, see http://pareview.sh/

This is a manual review...

Manual Review

Individual user account

[Yes: Follows] the guidelines for individual user accounts.

No duplication

[Yes: Does not cause] module duplication and/or fragmentation.

Master Branch

[Yes: Follows] the guidelines for master branch.

Licensing

[Yes: Follows / No: Does not follow] the licensing requirements.

3rd party assets/code

[Yes: Follows] the guidelines for 3rd party assets/code.

README.txt/README.md

[Yes: Follows] the guidelines for in-project documentation and/or the README Template.

Code long/complex enough for review

[Yes: Follows] the guidelines for project length and complexity.

Secure code

[Yes: Meets the security requirements. / No: List of security issues identified.]

Coding style & Drupal API usage

[List of identified issues in no particular order. Use (*) and (+) to indicate an issue importance. Replace the text below by the issues themselves:

No particluar issues were found. Issues identified by @sourabhutani in comment #11 seem to have been solved.

This review uses the Project Application Review Template.

Minor coding standard errors are not application blockers. Anything else that you found or should this be RTBC instead?

@klausi – are you asking me? If so, I would have wished for slightly clearer instructions on how to try it out and how to find the form #ID's. Had to fiddle around for a while before I got it to "kick in". It also seems the comma separated list of form #ID's doesn't tolerate spaces like:
my_form_id1, my_form_id2

this has to be
my_form_id1,my_form_id2

Perhaps a trim function or similar could be added when grabbing the variables to allow for space after comma.

EDIT: removed long pareview.sh dump, see http://pareview.sh/

Minor coding standard errors are not application blockers, please do a real manual review.

@sandipauti Yes, it works very well, but it may be even nicer if one could also have a blank space after the comma like this:
form_id2, form_id2, form_id3

when I tried the module it would only work if formated it like this:
form_id2,form_id2,form_id3

The reason is simply that I'm used to adding a blank space after a comma. So it is more an ease-of-use question and not a major thing...

Automated review

See attached file for formatting errors drupalcs-js_confirm_pop_up.txt

Manual review

The js-file doesn't load correctly:
GET http://loc.drupal7.dev/sites/all/modules/js_confirm_pop_up/js_confirm_pop_up.js?o44kzr 404 (Not Found)

This is due to line 43 in js_confirm_pop_up.module not having the correct path.
drupal_add_js(drupal_get_path('module', 'js_confirm_pop_up') . '/js_confirm_pop_up.js');

Should be
drupal_add_js(drupal_get_path('module', 'js_confirm_pop_up') . '/js/js_confirm_pop_up.js');

Either that or the js-file should be moved into the module's root directory

Hello sandipauti,

I didn't find any bug in your module, just you need to fix automated review report errors.
http://pareview.sh/pareview/httpgitdrupalorgsandboxsandipauti112680965git

Regards

Please don't RTBC your won issues, see the workflow: https://www.drupal.org/node/532400

Hi @sandipauti,

I have manually reviewed your code, Below are the findings,

• In .info file, package= Custom can be removed as it would be a contrib module after promoting to full project.
• The variable "js_confirm_pop_up_actual_form_id" is a configuration variable and should be removed/deleted on module un-installation.

There are a lot of examples available on https://www.drupal.org/node/1182224.

Removed the review bonus tag as you have not manually reviewed the modules.

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.

Hi sandipauti,

As per comment #39, please don't remove the single project promote tag.

As you also not reviewed other project application, removing the review bonus tag. Please review three other projects in the issue queue and update them in your issue description.

Adding the proper single project promotion tag.

Hi sandipauti,

very nice module, thanks.

In your js_confirm_pop_up_form_alter() you check for the existance of $form['#attributes']['class'] and then you are setting $form['#attributes'] = array('class' => array('js-confirm-pop-up'));. What happens if only the class attribute is not set but other attributes? You are deleting all the other attributes that might exist.

Best regards

How is this module different from the following:

• Node Edit Protection (either for D6 or D7).

  A very simple JavaScript based protection that will alert a user if they are navigating away from a modified Node Edit form without saving. It uses a standard JavaScript confirm dialog that allows the user to either continue what they were doing, or cancel the move allowing them the opportunity to save. It marks the form as dirty by using the blur event of the node fields.

• SaveGuard (either for D6 or D7).

  ... adds javascript which detects if a form had been modified, and if it has, uses the onBeforeUnload event to pop up a warning to the user if they attempt to navigate away from the page without saving their changes. This works for Firefox, IE, and Safari, but not Opera.

• Dirty Forms (only an official release for D6, though for D7 there seems to be a trustworthy patch available also).

  ... provides a javascript behavior that checks forms before users leave the page, warning them if they are about to loose changes and providing an option to remain on the page.

  ... The module provides a configuration form that allows setting up a list of forms that should be excluded from the dirtyForms behavior.

Source: http://drupal.stackexchange.com/a/160002/11723

@sandipauti, please don't remove the tags already added.

Hello Sandip,

Your module looks good. But there are some minor errors reported by automated review tools, did you check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxsandipauti112680965git

Regards,

Automated Review :

http://pareview.sh/pareview/httpgitdrupalorgsandboxsandipauti112680965gi...

Manual Review :

• js_confirm_pop_up.js : Drupal.js_confirm_pop_up = {}; Why you need this variable ? where are you using it ?
• js_confirm_pop_up_form_alter : Use #attached over drupal_add_js. see https://www.drupal.org/node/756722 http://drupal.stackexchange.com/questions/70015/adding-css-and-js-to-for...
• js_confirm_popup.module :
  

  /**
   * @file
   * Use to create configuration form.
   */

  Wrong file doc.it should be Main module file.

• Add more section in Readme.txt file like maintainers. Add about the features in features section.

Nothing blocker found. Looks good to me.
Assigning to klausi to give it a second look when he will get time.

Thank you for your reviews. When finishing your review comment also set the issue status either to "needs work" (you found some problems with the project) or "reviewed & tested by the community" (you found no major flaws).

manual review:

• project page is too short. what is the use case, why would I use the module? What are the differences to the other projects that asiby listed? See https://www.drupal.org/node/997024 . Can you add a screenshot?

Otherwise looks good to me. I think with the javascript this is just about our code limit, so I think we can approve you.

Thanks for your contribution, Sandip!

I updated your account so you can 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 stay 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.

Ah, that problem again, see #2726599: Sandbox repositories are occasionally not fully promoted & Git events are sometimes not recorded.