OZtree icon indicating copy to clipboard operation
OZtree copied to clipboard

Sponsorship_ends uses hardcoded duration value

Open hyanwong opened this issue 2 years ago • 3 comments

In pp_process_post, here:

https://github.com/OneZoom/OZtree/blob/eaed5526fa2705cb8fdf317558f7b7b9bd22c4a3/controllers/default.py#L1597

We should probably use days=sponsorship_config()['duration_days'] rather than days=365*4+1. In addition, it is fine to set this on PayPal response for renewals, but I wonder if it should be set on verification for new sponsorships. On the other hand, if this is likely to complicate the codebase substantially, then it's not worth the bother. I guess we could simply overwrite the sponsorship_end value on verification, which would make it nice and simple.

hyanwong avatar Apr 23 '23 15:04 hyanwong

In this line, we also set sponsorship_ends for renewals, so I'm not sure why we bother setting it in the pp_process_post function.

https://github.com/OneZoom/OZtree/blob/41fac8e29a2a3e31c313714947194bf79eb528e4/modules/sponsorship.py#L336

hyanwong avatar Apr 23 '23 15:04 hyanwong

@lentinj - do you think that we need to set sponsorship_ends in pp_process_post, for renewals? Or can we not set it there and move the code to set in when we verify a new sponsorship? From my reading, I think we can move setting sponsorship_ends like this if renewals are always via baskets.

hyanwong avatar Apr 23 '23 16:04 hyanwong

You have a point, but over 4 years I'm sure the few days taken to notice it needs verification could be forgiven.

If we move the update then there's potential that something will consider unverified sponsorships as permanently sponsored (since there's NULL sponsorship_ends). We could set it in both places though, which would also mean that if/when we change original sponsorships to use baskets we don't have to add any extra logic.

Extracting the verification step into modules/sponsorship would be nice though. Currently the unit tests have to bodge their own:

https://github.com/OneZoom/OZtree/blob/d7e25fa4441b20176113c5bac3de114c56e12d01/tests/unit/util.py#L205-L220

I'm not sure this is very feasible though, since we don't explicitly copy fields over. it happens as part of the form IIRC.

lentinj avatar Apr 24 '23 12:04 lentinj