search

svthalia / concrexit

Thalia Website built on Django.
https://thalia.nu
Other
22 stars 12 forks source link

Discount on membership upgrade triggers prevent_saving protection #3297

Closed DeD1rk closed 10 months ago

DeD1rk commented 1 year ago

Describe the bug

https://thalia.sentry.io/issues/4409801909

When people create a renewal to study-time membership, and a payment is made for it, the saving prevention is triggered because the contribution amount changed. The discount calculation should probably change to be calculated earlier so the amount is accurate from the start.

How to reproduce

Steps to reproduce the behaviour:

  1. Have an expiring membership
  2. Renew to study-long membership
  3. Pay
DeD1rk commented 1 year ago

I think this is what goes wrong:

  1. After creating a payment, the Renewal is saved to set Renewal.payment.
  2. Then, process_entry_save that responds to the payment having been set.
  3. In that, _create_membership_from_entry is called.
  4. If the current membership is still active at the date of the renewal (i.e. someone renews before September the 1st), the current membership until is set to None, and saved. So far, this is all fine.
  5. Now, entry.membership is set and saved (https://github.com/svthalia/concrexit/blob/22ff4c23b2373a9baaa0b904bde6ca95ff30f772/website/registrations/services.py#L391-L393), which is also still fine.
  6. But then, during that last save, entry.membership_discount_applies is evaluated again. This time however, it returns False, because member.current_membership.until is None (https://github.com/svthalia/concrexit/blob/22ff4c23b2373a9baaa0b904bde6ca95ff30f772/website/registrations/models.py#L127-L132). So then it thinks the contribution should change back to no discount.

So we should change the discount application to e.g. only be determined when entry.payment is None.

Also a note-to-self: I have to clean up any inconsistent entries in the production db at some point, as these and the other similar bugs have left behind e.g. 'Completed' entries without a linked payment, as well as paid entries that have been fully completed, but are still 'Accepted'

And a note to implementer: I would be forever grateful for a regression test for this :)