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.

Sign up for GitHub

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

Jump to bottom

Add printing adjustments #182

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Add printing adjustments #182

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4ea842c
Add printing adjustments
Jun 15, 2019
7ed21ed
Fix style
Jun 15, 2019
4f874b4
Incorporate feedback for adjustments
jyxzhang Oct 23, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 45 additions & 15 deletions ocflib/printing/ocfprinting.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,18 +16,19 @@ CREATE TABLE IF NOT EXISTS `jobs` (
PRIMARY KEY (`id`)
) ENGINE=InnoDB;

CREATE TABLE IF NOT EXISTS `refunds` (
CREATE TABLE IF NOT EXISTS `adjustments` (
`id` int NOT NULL AUTO_INCREMENT,
`user` varchar(255) NOT NULL,
`time` datetime NOT NULL,
`action` varchar(255) NOT NULL,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

`pages` int NOT NULL,
`staffer` varchar(255) NOT NULL,
`reason` varchar(510) NOT NULL,
PRIMARY KEY(`id`)
) ENGINE=InnoDB;

CREATE INDEX `jobs_idx` ON `jobs` (`user`, `time`, `pages`);
CREATE INDEX `refunds_idx` ON `refunds` (`user`, `time`, `pages`);
CREATE INDEX `refunds_idx` ON `adjustments` (`user`, `time`, `action`, `pages`);

DROP FUNCTION IF EXISTS semester_start;
DELIMITER $$
Expand All @@ -52,30 +53,58 @@ CREATE VIEW jobs_today AS
DROP VIEW IF EXISTS refunds_today;
CREATE VIEW refunds_today AS
SELECT user, SUM(pages) AS pages
FROM refunds
WHERE DATE(refunds.time) = CURDATE()
FROM adjustments
WHERE DATE(adjustments.time) = CURDATE()
AND action = "refund"
GROUP BY user;

DROP VIEW IF EXISTS forwards_today;
CREATE VIEW forwards_today AS
SELECT user, SUM(pages) AS pages
FROM adjustments
WHERE DATE(adjustments.time) = CURDATE()
AND action = "forward"
GROUP BY user;

DROP VIEW IF EXISTS printed_today;
CREATE VIEW `printed_today` AS
SELECT
jobs_today.user AS user,
COALESCE(jobs_today.pages, 0) - COALESCE(refunds_today.pages, 0) AS today
COALESCE(jobs_today.pages, 0) - COALESCE(refunds_today.pages, 0)
- COALESCE(forwards_today.pages, 0) AS today
FROM jobs_today
LEFT OUTER JOIN refunds_today
ON jobs_today.user = refunds_today.user
LEFT OUTER JOIN forwards_today
ON jobs_today.user = forwards_today.user
GROUP BY jobs_today.user

UNION

SELECT
refunds_today.user AS user,
COALESCE(jobs_today.pages, 0) - COALESCE(refunds_today.pages, 0) AS today
FROM jobs_today
RIGHT OUTER JOIN refunds_today
ON jobs_today.user = refunds_today.user
COALESCE(jobs_today.pages, 0) - COALESCE(refunds_today.pages, 0)
- COALESCE(forwards_today.pages, 0) AS today
FROM refunds_today
LEFT OUTER JOIN jobs_today
ON refunds_today.user = jobs_today.user
LEFT OUTER JOIN forwards_today
ON refunds_today.user = forwards_today.user
GROUP BY refunds_today.user

UNION

SELECT
forwards_today.user AS user,
COALESCE(jobs_today.pages, 0) - COALESCE(refunds_today.pages, 0)
- COALESCE(forwards_today.pages, 0) AS today
FROM forwards_today
LEFT OUTER JOIN jobs_today
ON forwards_today.user = jobs_today.user
LEFT OUTER JOIN refunds_today
ON forwards_today.user = refunds_today.user
GROUP BY forwards_today.user

ORDER BY user;

DROP VIEW IF EXISTS jobs_semester;
Expand All @@ -88,8 +117,9 @@ CREATE VIEW jobs_semester AS
DROP VIEW IF EXISTS refunds_semester;
CREATE VIEW refunds_semester AS
SELECT user, SUM(pages) AS pages
FROM refunds
WHERE DATE(refunds.time) >= semester_start(CURDATE())
FROM adjustments
WHERE DATE(adjustments.time) >= semester_start(CURDATE())
AND action = "refund"
GROUP BY user;

DROP VIEW IF EXISTS printed_semester;
Expand All @@ -107,9 +137,9 @@ CREATE VIEW `printed_semester` AS
SELECT
refunds_semester.user AS user,
COALESCE(jobs_semester.pages, 0) - COALESCE(refunds_semester.pages, 0) AS semester
FROM jobs_semester
RIGHT OUTER JOIN refunds_semester
ON jobs_semester.user = refunds_semester.user
FROM refunds_semester
LEFT OUTER JOIN jobs_semester
ON refunds_semester.user = jobs_semester.user
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why was this change made?

GROUP BY refunds_semester.user

ORDER BY user;
Expand All @@ -135,4 +165,4 @@ CREATE VIEW `public_jobs` AS
ORDER BY `day` DESC

GRANT SELECT ON `ocfprinting`.`printed` TO 'anonymous'@'%';
GRANT SELECT on `ocfprinting`.`public_jobs` TO 'anonymous'@'%';
GRANT SELECT ON `ocfprinting`.`public_jobs` TO 'anonymous'@'%';
7 changes: 4 additions & 3 deletions ocflib/printing/quota.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,10 +45,11 @@
'filesize',
))

Refund = namedtuple('Refund', (
Payload = namedtuple('Payload', (
'user',
'time',
'pages',
'action'
'staffer',
'reason',
))
Expand Down Expand Up @@ -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."""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change this docstring

c.execute(*_namedtuple_to_query('INSERT INTO refunds ({}) VALUES ({})', refund))
c.execute(*_namedtuple_to_query('INSERT INTO adjustments ({}) VALUES ({})', payload))