pgeu / pgeu-system

Mirror of the PostgreSQL Europe Conference and Membership management system.
MIT License
20 stars 29 forks source link

Allow sponsorship payment terms(due date) to be changed #170

Closed ssinger closed 1 day ago

ssinger commented 2 weeks ago

Allow the payment terms(days until payment to be paid) set to values other than 30. Also allow a deadline to be set where payment is due at the deadline(or on the invoice date) after this time.

mhagander commented 1 week ago

I like the principle f this, but a couple of notes.

  1. docs :)
  2. wouldn't it be easier to just always store the number of days for the invoice in the db, with a default value of 30? That would remove a bunch of special-casing at runtime, I think?
  3. I'm not sure about the terms used? To me, "payment deadline" would be the date that this patch calls "payment terms", and the other way around. Doesn't mean it's right but it felt backwards to me, so maybe see if we can find some other terms that make it more clear.
  4. Do we intentionally want to allow deadlines both before and after the actual conference date?
ssinger commented 1 week ago
  1. I've added a section to the sponsor docs
  2. I've set a default of 30 days for the terms, it didn't remove that much runtime logic
  3. I've replaced "payment deadline" with "paymentdueby" does that make things clearer?
  4. Yes I think we should allow deadlines after the conference date if explicitly picked as a deadline. If someone wants to do this explicitly the system shouldn't stop them they might have reasons (prioritizing sponsors with $x day payment terms over cashflow and collection leverage). I've adjusted the invoicehandler to make sure this works
mhagander commented 4 days ago

This looks a lot better, I think.

Reading it again and thinking some more, I thought of one more thing. Should we not just get rid of the "5 days" rule as well, and instead just default the due-by date to 5 days before the conference? ISTM if we're making it flexible we should turn as much hard-coding into default-but-changeable instead. In doing this we could say that the due date is mandatory, and just default it to 5 days before, which I think would make the process clearer?

Also, should we not make paymentdueby be a DateField rather than a DateTimeField and set the time to midnight? I think most people are used to dealing with invoice due dates rather than times. In fact we already present it as a date rather than time in at least some views. This would also make it match up cleaner with the start date of a conference which is defined as a date rather than a time.

ssinger commented 4 days ago

I've updated paymentdueby to be just a date and be mandatory

mhagander commented 1 day ago

I took the liberty to change paymentterms to paymentdays in the model -- you immediately turned it into a variable named that, and the documentation also named it that, so it seemed simpler.

I also fixed a couple of typos, and updated the documentation for th elatest changes you made (making it mandatory).

I changed the at least to me strange combination of "nulls not allowed but blanks allowed" for a date field (what's a blank date?), and set not null on the int field since it's supposed to be mandatory (and was when edited through django, but we want the not null constraint).

Added an actual default for the paymentdueby field - you only had it setting the default in the migration, not when actually creating a new level.

Oh, and I rebased it on master. Now let's see if I can figure out how to actually update the PR...