Add printing adjustments #182
Conversation
|
The associated quota tests need to be updated as well before merging this in |
ocflib/printing/quota.py
Outdated
| @@ -117,6 +118,6 @@ def add_job(c, job): | |||
| c.execute(*_namedtuple_to_query('INSERT INTO jobs ({}) VALUES ({})', job)) | |||
|
|
|||
|
|
|||
| def add_refund(c, refund): | |||
| def add_adjustment(c, payload): | |||
| """Add a new refund to the database.""" | |||
ocflib/printing/ocfprinting.sql
Outdated
| `id` int NOT NULL AUTO_INCREMENT, | ||
| `user` varchar(255) NOT NULL, | ||
| `time` datetime NOT NULL, | ||
| `action` varchar(255) NOT NULL, |
There was a problem hiding this comment.
This should either be an enum (refund or forward), or you should leave this table alone and make a new forwards table. Personally I think the latter is a better option, since every time you use this table you have to check action anyways. But either is fine.
There was a problem hiding this comment.
I was initially considering to go with the latter option that you proposed, but I ultimately decided to continue with this current implementation to introduce less complexity for the next person who decides to work on this section of the code.
Performance-wise, I believe that the latter option will be better off as well due to locality, but it is hard to tell for certain because of how SQL operates on the lower layers.
There was a problem hiding this comment.
We don't need to care about performance for this. Why do you think your current solution introduces less complexity? It's slightly less verbose but I would argue needing to put if statements everywhere to check for refund/forward makes things more complicated for anyone working on this.
There was a problem hiding this comment.
I actually think one table is logically and conceptually simpler. We never interact with this table directly anyways on the read, we do it through the views, which introduce the two table abstraction you're looking for. My conception of this problem is that the difference between refunds and adjustments is whether they permanently increase semesterly quota when increasing daily quota, and given that that is an application-code level concern that just requires a binary switch in the database to decide, it suggests they should continue to live in the same table.
Would switch the column for a enum, or better yet, a boolean, though. eg is_adjustment or is_refund
| ON jobs_semester.user = refunds_semester.user | ||
| FROM refunds_semester | ||
| LEFT OUTER JOIN jobs_semester | ||
| ON refunds_semester.user = jobs_semester.user |
Depended on by ocf/utils#114
Allows staff members to adjust users' paper quotas in a more flexible manner.
We add the following functionality to the
paperscript:forward: increases a user's daily quota for the day onlyThis is intended to be used to give users some degree of freedom when printing documents.
For example, if the daily print limit was set at
10pages and a user wanted to print12pages, this would not be possible with the current way things are configured.forwardprovides the way to increase the daily print limit without increasing the amount of pages a user can print in a semester.