-
Notifications
You must be signed in to change notification settings - Fork 0
ETT-298: Additional column for full download #271
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,7 +14,7 @@ class HTDownload < ApplicationRecord | |
| scope :for_role, ->(role) { where(role: role) } | ||
|
|
||
| def self.ransackable_attributes(auth_object = nil) | ||
| ["role", "datetime", "email", "htid", "id", "in_copyright", "inst_code", "is_partial", "sha", "yyyy", "yyyymm", "pages"] | ||
| ["role", "datetime", "email", "htid", "id", "in_copyright", "inst_code", "sha", "yyyy", "yyyymm", "full_download", "pages"] | ||
| end | ||
|
|
||
| def self.ransackable_associations(auth_object = nil) | ||
|
|
@@ -33,16 +33,16 @@ def hf | |
| ht_hathifile | ||
| end | ||
|
|
||
| def partial? | ||
| is_partial | ||
| def full_download | ||
| !is_partial | ||
| end | ||
|
|
||
| ransacker :datetime do | ||
| Arel.sql("DATE(#{table_name}.datetime)") | ||
| end | ||
|
|
||
| ransacker :pages do | ||
| Arel.sql("COALESCE(#{table_name}.pages, CASE WHEN #{table_name}.is_partial = '0' THEN 'all' ELSE 'unknown' END)") | ||
| ransacker :full_download do | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't love that the logic is kind of duplicated between the ransacker and the presenter but that may just be what we need to do right now. We may want to rethink some of this later if we end up displaying the report in Tableau or something long-term (which might let us unwind some of what we're doing in the presenter)
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could consider inverting the Boolean in the database, transition is_partial to is_full or whatever. But not now.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed, not now. I created an issue: https://hathitrust.atlassian.net/browse/ETT-745 |
||
| Arel.sql("(CASE WHEN #{table_name}.is_partial = '0' THEN 'yes' WHEN #{table_name}.is_partial = '1' THEN 'no' END)") | ||
| end | ||
|
|
||
| def sha | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The bang makes this a reliable Boolean, so I would rename to
full_download?. However the bareis_partialwill return 0 or 1 so I think we need... to avoid Ruby's "zero is true" maldesign
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is confusing because I see a difference between k8s and Docker when I look at the
is_partialmethod:bundle exec rails con otis-testing gives 0/1 where in Docker I'm getting true/false. Our schema is doing the wrong thing, if you change db/schema.rb so is_partial looks liket.integer :is_partial, limit: 1 # TINYINT(and if necessary change the seeds to use [1,0].sample instead of bools) -- the display will always be "no" for the Full Download column (i.e., unless you swap in thefull_download?method in the above comment).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know why the
t.booleanis being ignored in k8s. But I'm pretty sure the correct thing to do is make it so we always expect an integer fromis_partialand a bool fromis_partial?.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will take a look at this in Kubernetes. I believe this has to do with table columns being declared as
tinyint(1)vsint(1)but I'm not certain. I don't think we needis_partial?at all, and I think I ran into an issue trying to use an attribute with?in it with the table display & ransacker stuff but I'll look again. I do agreefull_download?would be preferable if possible.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Verified using
tinyint(1)works.