Skip to content

fix: Fully implement fix from !102, including hardening of test suite

stephen requested to merge propagate-regression-fix-to-perk-validation into main

This commit continues the work of MR !102 (merged), which applied the right fix but did not go far enough.

In that MR, we ensured that, after fixing the regression where custom_donation was being ignored instead of leveraged, backend validation of donations was checking minimum donation values by comparing dollars to dollars, instead of dollars to cents. We then adjusted the test suite governing the donation form to ensure that the default custom_donation value used there reflected the values we'd actually find.

Validation is also performed against perk donation minimums, and tests are also performed against that validation. And as it happens, perk donation minimums also needed to be adjusted in order to properly compare dollars to cents - but !102 (merged) did not catch this need. (User testing did!)

In the interest of preventing any more cat-herding around this regression, this MR does fix the aforementioned issue with the form validation (thereby resolving the issue), but more importantly, it updates the unit test suite in the following ways:

  • All donation-amount test values are prepared in tordonate.tests.test_donor_form.setUp(), where they are drawn directly from the civicrm repository (which gets that data from source)
  • Tests against donation amounts have both negative and positive tests
  • Positive tests against donation amounts are held by donating the precise minimum
  • Negative tests against donation amounts are held by donating the precise minimum - 1
  • All donation form combinations (vendor, frequency, perk) have been re-tested from the front-end
  • All donation form unit tests have, themselves, been tested
  • Because some of the confusion leading to !102 (merged) being incomplete stemmed from the minimum donation value being defined in a totally different place than other statically-defined price values - a holdover from when it was mock data - settings.MINIMUM_DONATION has been added to settings.py alongside DONATE_PRICES and DEFAULT_DONATION, and its value is now set there, and referenced (rather than set) by containers.py.
  • Comments have been added throughout.

It is worth mentioning for the sake of future Gitlab perusers that #68 can and should be implemented in a way where custom_donation contains a numeric "cents" value (and is displayed to the user as a USD-formatted value, whether that be "dollars" or "dollars.cents"). This would allow us to compare cents to cents throughout the backend and would allow us to refactor out all of this fussy conversion math.

Merge request reports