Fix IBAN conversion validation #588
Conversation
|
This reintroduces phab:T140476, which should have been fixed by #568, but apparently wasn't. |
The forms were sending empty fields to the server, sometimes even incorrect fields (See https://phabricator.wikimedia.org/T141381). This was introduced by #568, which was not the correct fix for the issue.
gbirke
changed the title
|
I supposed Kai's comment has been addressed since the commit is more recent now... +1 as I don't see anything obviously wrong, though then I don't understand the code well enough to catch a lot of things. If this is something that was wrong and then not fully fixed and reintroduced, having a test for it seems real nice. Is that difficult to do nicely in this case? |
| @@ -97,6 +100,10 @@ var jQuery = require( 'jquery' ), | |||
| }; | |||
| validationUrl = this.validationUrlForNonSepa; | |||
| } | |||
| // avoid sending incomplete data to the server | |||
| if ( _.find( data, isEmptyString ) !== undefined ) { | |||
There was a problem hiding this comment.
It took me a good number of seconds to fully interpret what this is doing, promoting me to create f6c2b02. As it is here, you need to deal with not relevant low level details such as the behaviour of the _.find function.
As I found out there, there is no need for such a general check, or for the local variables in this method. 646dcc9
PR: #591
Reduced state and made checks more explicit in BankDataValidator
|
I think for testing this behavior, you really need a browser test, that simulates a user clicking in the interface. |
|
There is one tiny bit missing: If I switch from direct debit to any other payment method, the bank data fields are empty and receive a green validity indicator in all cases (were valid before, were invalid before, were already empty). |
Apart from the `OK` and `ERR` states we had `INCOMPLETE`. `Now they are all bundled up in a value object. For the bank data validation the state `NOT_APPLICABLE` was added to avoid sending data to the server and changing the validation state when a different payment method thank direct debit is selected.
|
Addressed comment from @KaiNissen @JeroenDeDauw Could you merge this? |
Fixes https://phabricator.wikimedia.org/T141381