Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 11 commits into from

Conversation

ssinger
Copy link
Contributor

@ssinger ssinger commented Nov 10, 2024

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
Copy link
Member

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
Copy link
Contributor Author

ssinger commented Nov 15, 2024

  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
Copy link
Member

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
Copy link
Contributor Author

ssinger commented Nov 21, 2024

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

@mhagander
Copy link
Member

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...

@mhagander mhagander force-pushed the sponsorship-terms branch 2 times, most recently from a1a7f41 to 643c414 Compare November 24, 2024 08:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants