developer.jelix.org is not used any more and exists only for history. Post new tickets on the Github account.
developer.jelix.org n'est plus utilisée, et existe uniquement pour son historique. Postez les nouveaux tickets sur le compte github.

Opened 12 years ago

Closed 12 years ago

Last modified 12 years ago

#751 closed bug (fixed)

jForms: javascript check of secret control's confirmation does not work

Reported by: Julien Owned by: Julien
Priority: normal Milestone: jelix 1.1
Component: jelix:forms Version: trunk
Severity: normal Keywords:
Cc: Blocked By:
Blocking: Documentation needed: no
Hosting Provider: Php version:

Description

It didn't work because generated JS used jFormsJQControlString instead of jFormsJQControlSecret.

Now Javascript uses a generic confirm control, that may be used for other controls than <secret> (maybe we could introduce <confirm> for <input> controls in the future, to confirm an email for example)

Also updated unit tests.

NB : in the future, I think we'll need the ability to set errRequired, errInvalid and help messages for the confirmation control (currently they are inherited from the "parent" control when specified). But this will be another ticket.

Attachments (1)

Change History (10)

comment:1 Changed 12 years ago by Julien

  • Status changed from new to assigned

comment:2 Changed 12 years ago by laurentj

  • review changed from review? to review-

I don't think you have to change all this things to fix the bug. We are in a "features freeze" state (yes , I have to inform all developers of that :-)), so only bug fix are accepted for 1.1.

Please propose a patch to fix only the bug, and create a ticket to add the support of the confirm element on input elements.

I don't want to break something just before the RC1...

comment:3 Changed 12 years ago by Julien

I'm not sure this could be simplier, because with the old way in JS check of secret control with confirmation, the alert messages were not good:

if(this.confirmField){ 
    var val2 = jfrm.getValue(jfrm.frmElt.elements[this.confirmField.name]); 
    if(val != val2){ 
        jfrm.tForm.errorDecorator.addError(this.confirmField, 2);
        // this line adds an error message equal to "this.confirmField.errInvalid", which is empty.
        return false;
        /*
        returning false will add a error message equal to this.errInvalid,
        which is not really good, because it's the confirmation which is invalid,
        not the "master" control itself.
        */
    } 
}

So we have the following messages in the alert box:

  • [* ] (blank message because this.confirmField.errInvalid is empty)
  • [* the field "password" is invalid] (instead of: the "confirm password" field is invalid)

That's why I made all theses changes, and made the confirmation control a standard control, which is checked like all the others.

I'm not sure I can have a better approach to solve this bug, so please consider taking a look again at the patch.

NB: this patch only solves the bug, there is not implementation for the confirmation on <input> controls, nor grammar enhancement for the <confirm> tag (to specify proper alert messages, etc...). That will be another ticket, as I said.

comment:4 Changed 12 years ago by Julien

  • review changed from review- to review?

comment:5 Changed 12 years ago by laurentj

After re-reading the patch, I'm ok for the principle. But I think we can simplify the code :

+ this._masterControl = jForms.tForm.getControl(name.replace(/_confirm$/,'')); 

Perhaps this following code is enough ?

+ this._masterControl = name.replace(/_confirm$/,''); 

Do we really need the object of the control, since we need only its name ?

comment:6 Changed 12 years ago by Julien

you're right, we only need the name, will change it when committing if everything else is OK

comment:7 Changed 12 years ago by laurentj

  • review changed from review? to review+

ok :-)

comment:8 Changed 12 years ago by Julien

  • Resolution set to fixed
  • Status changed from assigned to closed

committed in the trunk, r1188

will backport it to 1.0.x branch too

comment:9 Changed 12 years ago by Julien

finally, it works right in 1.0.x

Note: See TracTickets for help on using tickets.