Skip to content
This repository has been archived by the owner on Jul 31, 2023. It is now read-only.

Banner template validation #12

Closed
wants to merge 17 commits into from
Closed

Conversation

gbirke
Copy link
Member

@gbirke gbirke commented Oct 15, 2015

Add validation code that connects the sensitive banner to the server for validation and data submission.

@wmde-manicki
Copy link

Generally looks really good. Data is validated, if all is good then user is transferred to the donation website. Donation data is stored in the database. (Tested on my local instances but there is also test running on test.wikimedia.de, so I assume we can say that it works).
Below some remarks that I've noticed:

  • this depends on banner-template-validation for obvious reasons. I'd like to merge static banner upgrade form #3 quite soon (actually even right now). Could you make this a pull request onto master once that PR is merged? It'd also make reviewing easier when there is a single actual PR instead of two monstrous PRs combined into one
  • on the SEPA agreement page (second step of the Lastrschrift donation) links to change address data, payment data and the amount do not work - this might not be issue in the scope of this particular PR but should definitely not be forgotten
  • Error boxes are there when I hide the form and open it again - this is actually good. I am just wondering whether error box regarding amount should actually hide when hiding the form?
  • another thing regarding the the error box about the invalid amount - it covers the Lastschrift button. I don't see any better place to put the error message so it wouldn't hide anything else. Maybe there should be some way to hide the particular (or all) errors? How about discussing this with Henning?
  • also the thing to consider, possibly not requiring a change: maybe the message about inserting valid amount when e.g. entering 0.50 when donating with credit card is not that informational? Maybe it should explicitly say what amount could be entered?
  • Looking for it briefly in the code I have not noticed that it is requiring user to tick the fields in SEPA withdrawal agreement page (SEPA-Mandat)
  • is donating with direct debit (Lastschrift) actually possible right now? I did not notice BIC (and other bank data) being "generated" from the IBAN (this might not be in the scope of this PR), but also nothing seem to happen when I fill data in all fields of direct debit donation and click the yellow button on both steps. It just folds in the form. Same actually happens when I enter no data and just click "proceed". It might be something with my dev instance as some strange things started to happen while I was testing this. So if you say that this payment method should work as good as others, I'll try it again.
  • I'd really not want to merge any pull request that is failing on CI. Especially when those JSHint complaints seem sane. Could you please fix those? At least by adding a commit fixing the issue.

manicki and others added 17 commits October 16, 2015 10:24
At the moment, the GPG public key is not configurable, this must be
changed before deployment!
Moved getting the amount from the form to its own function.
Use the function to validate in the Banner class => not ideal, needs to
be moved/refactored later.
Instead of adding and removing CSS classes we now trigger custom jQuery
events. This makes the handling of venets more flexible.
The sensitive banner now uses thze validation events.
Since the Banner class is sensitive banner specific it makes more sense
to put markup-specific code in the Banner.Form class.
Otherwise the value "Kein Titel" would be sent which is not a valid
value.
Error (and success) markup is now removed before validating values.
Special code for validating the amount.
The validation only succeeds if the data that is sent to the server
matches the selected payment type and address type, so we must send the
correct data on the client side.
When payment type or address type changes, the fields that are no
longer needed are cleared.
The code was already setting the payment type, no need to check the
button status.
Simulate the button on the original donation form which encodes the form
action and the payment type.
getFormData returned jQuery object instead of value.
Form now sends data to regular donation page on validation success.
Move them a bit to the left so they don't break
@gbirke
Copy link
Member Author

gbirke commented Oct 19, 2015

Thanks for all the comments.
I've talked to Henning about the obscured buttons and we decided to leave them obscured for the time being.
I've tried to give valid amounts in the error message but this will make the tooltip to long, breaking it over two lines and generally boing very ugly. Don't know how to solve that.
I've created a new commit that fixes the JSHint violations.

I will close this PR now and create a new one against master as soon as I have addressed the other points (SEPA mandate validation and BIC generation)

@gbirke gbirke closed this Oct 19, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants