Introduce jquery formvalidator for com_users #5058

dgrammatiko · 2014-11-08T17:53:21Z

Executive summary

This PR converts the form validation on com_users to use plain jquery (no mootools call on every form ).
Also NO MORE INLINE SCRIPTS!

Testing

Apply first PR Reduce unneeded calls for mootools #4888 (!important)
Apply this PR
In the admin area go to com_users and try to submit any form.

If no javascript errors are logged in your browser and the functionality remains the same your test is a pass in any other case please report the errors here

smanzi · 2014-11-11T01:22:02Z

@test success

anibalsanchez · 2014-11-13T13:14:26Z

@test success!

This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/5058.

dgrammatiko · 2014-11-13T13:36:29Z

@smanzi @anibalsanchez Thank you very much for testing all these PRs. I owe you some beers 🍻

smanzi · 2014-11-13T20:52:07Z

@DGT41 @anibalsanchez That would be great! 🍺

roland-d · 2014-11-18T18:36:06Z

Moving to RTC as we have 2 successful tests.

This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/5058.

phproberto · 2014-11-20T09:50:29Z

administrator/components/com_users/views/groups/tmpl/default.php

Can we please avoid this $script .= thing? You do it right later with:

JFactory::getDocument()->addScriptDeclaration(" // JS code here ");

So it's easier to modify it or move it to a js file.

Thanks!

@phproberto I also dislike $script .= or $script[] = but in this occasion there is a foreach loop that populates some part of the script. I guess I can simplify it to three parts:

code before the foreach

code generated in the loop

the rest

Or use JFactory::getDocument()->addScriptDeclaration(“…”); 3 times (i am not sure if this is gonna work flawlessly)

@phproberto is this ok?

$script = ' jQuery(document).ready(function() { Joomla.submitbutton = function(task) { if (task == "groups.delete") { var f = document.adminForm; var cb="";'; foreach ($this->items as $i => $item) { if ($item->user_count > 0) { $script .= ' cb = f["cb"+' . $i . ']; if (cb && cb.checked) { if (confirm(Joomla.JText._("COM_USERS_GROUPS_CONFIRM_DELETE"))) { Joomla.submitform(task); } return; }'; } } $script .= ' } Joomla.submitform(task); } });'; JFactory::getDocument()->addScriptDeclaration($script);

Just my 2cents, I agree the $script .= is not ideal but the former looks more readable to me than the latter.

@DGT41 please see my PR against your branch: https://github.com/dgt41/joomla-cms/pull/10

@phproberto thanks! there are also some bad $script .= on admin com_languages and com_menus, I will try to fix them tomorrow.
I'm on the iPhone right now so can't do much

If other files using $script .= are not related to this PR I think we can do that on a different PR and merge this.

phproberto · 2014-11-20T23:34:56Z

I think we need at least an extra test to validate that my latest changes to the group management work.

How to test it

When trying to delete a group with users inside you should see an alert box asking you confirmation
When trying to delete a group with no users you should be able to delete it without confirmation

Fedik · 2014-11-21T09:06:04Z

test
works good for me

anibalsanchez · 2014-11-21T13:02:24Z

@test OK

Group deletion without users, Ok
Group deletion with users, standard alert, confirmation Yes, OK (group deleted)
Group deletion with users, standard alert, confirmation No, OK (group not deleted)

dgrammatiko · 2014-11-21T13:55:21Z

@Fedik @anibalsanchez Thanks, your help is really appreciated. Can I ask you to check also, if you have spare time,
#5047 (administrator/components/com_menus/views/item/tmpl/edit_modules.php AND administrator/components/com_menus/views/menutypes/tmpl/default.php)
#5046 (administrator/components/com_languages/views/override/tmpl/edit.php)

smanzi · 2014-11-25T02:16:38Z

@test success (all groups deletion conditions too...)

Just be a little bit picky, I hate alert messages (the "standard" ones), also because they can be switched off at the browser level. I think we should design our own modal-based alert/confirmation message...

smanzi · 2014-11-30T02:08:58Z

@test success for this PR
Tested the set of: #5046 #5048 #5049 #5050 #5051 #5052 #5053 #5054 #5056 #5058 #5113 #5255

smanzi · 2014-11-30T02:12:55Z

~~@test success for this PR (created new article in front-end, no prob!)~~
~~Tested the set of: #5046 #5048 #5049 #5050 #5051 #5052 #5053 #5054 #5056 #5058 #5113 #5255~~

smanzi · 2014-11-30T02:13:25Z

sorry, misplaced comment above!

This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/5058.

dgrammatiko · 2014-11-30T02:53:59Z

The strange thing is that in commit https://github.com/dgt41/joomla-cms/commit/9999882bcbce06b6adc24c13f4b2936c3f24c01a the code is there and disappears out of the blue!
Can we test it one more time?

dgrammatiko · 2014-11-30T02:56:44Z

@smanzi @phproberto here is ok!

smanzi · 2014-11-30T03:00:07Z

here too! e.g.: the patch downloaded by com_patchtester has at line 26-31 of administrator/components/com_users/views/groups/tmpl/default.php:

$script = '
    Joomla.submitbutton = function(task) {
        if (task == "groups.delete")
        {
            var f = document.adminForm;
            var cb="";';

dgrammatiko · 2014-11-30T03:01:27Z

Yeah I pushed the code again…

smanzi · 2014-11-30T03:02:53Z

... but I didn't reapply the patch: this is from my original patch that applied when started testing tonight...

smanzi · 2014-11-30T03:04:27Z

do you want me to re-apply & re-test this?

dgrammatiko · 2014-11-30T03:05:06Z

if you can but give me a minute

smanzi · 2014-11-30T03:06:09Z

no prob, give me the "go"...

dgrammatiko · 2014-11-30T03:09:42Z

go!

dgrammatiko · 2014-11-30T03:13:30Z

Is looking good here!

smanzi · 2014-11-30T03:13:51Z

Yes, now code different and anyway @test success!

phproberto · 2014-11-30T03:18:35Z

administrator/components/com_users/views/user/tmpl/edit.php

We have to remove this one too.

but this is jquery dependent ? jQuery('#jform_twofactor_method').val();

If it was attached to an element's event it should be done on document.ready but this is only a function declaration that is called by other elements (in this case is fired when a select box changes) so it's done when the DOM is ready.

Also this code was executed like that originally so let's keep it. We have enough problems :D

phproberto · 2014-11-30T04:07:51Z

Thanks. All seems ok now. We need to wait to travis so I'll go to sleep.

You did an awesome work tonight. Thanks!!

smanzi · 2014-11-30T04:09:02Z

again @test success!

smanzi · 2014-11-30T04:11:04Z

Good night, everybody! 😴

dgrammatiko · 2014-11-30T04:22:36Z

Its time 💤

Fedik · 2014-11-30T14:13:29Z

test
works good for me

anibalsanchez · 2014-12-01T13:56:31Z

@test OK

New user,mandatory fields
User Modification, mandatory fields
Group deletion without users, Ok
Group deletion with users, standard alert, confirmation Yes, OK (group deleted)
Group deletion with users, standard alert, confirmation No, OK (group not deleted)

Bakual · 2014-12-02T20:09:19Z

Merged into staging. Thanks!

jissues-bot added the PR-staging label Nov 8, 2014

dgrammatiko mentioned this pull request Nov 9, 2014

Emoticons break descriptions joomla/jissues#556

Closed

dgrammatiko mentioned this pull request Nov 15, 2014

Reduce unneeded calls for mootools #4888

Closed

roland-d added RTC This Pull Request is Ready To Commit and removed RTC This Pull Request is Ready To Commit labels Nov 18, 2014

jissues-bot added the RTC This Pull Request is Ready To Commit label Nov 18, 2014

phproberto reviewed Nov 20, 2014
View reviewed changes

dgrammatiko mentioned this pull request Nov 21, 2014

Introduce jquery formvalidator for com_menus #5047

Closed

dgrammatiko mentioned this pull request Nov 30, 2014

Introduce jquery formvalidator for com_languages #5046

Closed

dgrammatiko added 5 commits November 30, 2014 04:47

Quotes

2a8d6ae

fix

6a66dac

Respect the CS

3a96ce5

CS

9999882

.ready()

d58b6ac

Roberto's code

34dd8c7

phproberto reviewed Nov 30, 2014
View reviewed changes

.ready()

192ba36

Bakual added this to the Joomla! 3.4.0 milestone Dec 2, 2014

Bakual closed this in fe29555 Dec 2, 2014

dgrammatiko deleted the _form_jq_com_users branch December 2, 2014 21:16

zero-24 removed the RTC This Pull Request is Ready To Commit label Oct 14, 2015

Uh oh!

Introduce jquery formvalidator for com_users #5058

Introduce jquery formvalidator for com_users #5058

Uh oh!

Conversation

dgrammatiko commented Nov 8, 2014

Executive summary

Testing

Uh oh!

smanzi commented Nov 11, 2014

Uh oh!

anibalsanchez commented Nov 13, 2014

Uh oh!

dgrammatiko commented Nov 13, 2014

Uh oh!

smanzi commented Nov 13, 2014

Uh oh!

roland-d commented Nov 18, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phproberto commented Nov 20, 2014

How to test it

Uh oh!

Fedik commented Nov 21, 2014

Uh oh!

anibalsanchez commented Nov 21, 2014

Uh oh!

dgrammatiko commented Nov 21, 2014

Uh oh!

smanzi commented Nov 25, 2014

Uh oh!

smanzi commented Nov 30, 2014

Uh oh!

smanzi commented Nov 30, 2014

Uh oh!

smanzi commented Nov 30, 2014

Uh oh!

dgrammatiko commented Nov 30, 2014

Uh oh!

dgrammatiko commented Nov 30, 2014

Uh oh!

smanzi commented Nov 30, 2014

Uh oh!

dgrammatiko commented Nov 30, 2014

Uh oh!

smanzi commented Nov 30, 2014

Uh oh!

smanzi commented Nov 30, 2014

Uh oh!

dgrammatiko commented Nov 30, 2014

Uh oh!

smanzi commented Nov 30, 2014

Uh oh!

dgrammatiko commented Nov 30, 2014

Uh oh!

dgrammatiko commented Nov 30, 2014

Uh oh!

smanzi commented Nov 30, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!