ETT-298: Additional column for full download #271
Conversation
|
|
||
| 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 |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Could consider inverting the Boolean in the database, transition is_partial to is_full or whatever. But not now.
There was a problem hiding this comment.
Agreed, not now. I created an issue: https://hathitrust.atlassian.net/browse/ETT-745
|
|
||
| def partial? | ||
| is_partial | ||
| def full_download |
There was a problem hiding this comment.
The bang makes this a reliable Boolean, so I would rename to full_download?. However the bare is_partial will return 0 or 1 so I think we need
def full_download?
!is_partial?
end
... to avoid Ruby's "zero is true" maldesign
There was a problem hiding this comment.
This is confusing because I see a difference between k8s and Docker when I look at the is_partial method: bundle exec rails c on 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 like t.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 the full_download? method in the above comment).
There was a problem hiding this comment.
I don't know why the t.boolean is being ignored in k8s. But I'm pretty sure the correct thing to do is make it so we always expect an integer from is_partial and a bool from is_partial?.
There was a problem hiding this comment.
I will take a look at this in Kubernetes. I believe this has to do with table columns being declared as tinyint(1) vs int(1) but I'm not certain. I don't think we need is_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 agree full_download? would be preferable if possible.
There was a problem hiding this comment.
Verified using tinyint(1) works.
moseshll
left a comment
moseshll
left a comment
There was a problem hiding this comment.
I have a patch that should take care of the is_partial issues, I won't muddy the waters by committing the changes but I've got them at the ready if needed.
|
Regarding the requested changes:
Let me know if that's all OK and then I think we should be good to merge. |
|
I will go ahead and rebase/resolve the conflict. |
* additional custom ransacker for full download * remove custom ransacker for pages
aelkiss
force-pushed
the
ETT-298-two-columns
branch
from
c984a8e to
ad24413
Compare
moseshll
left a comment
moseshll
left a comment
There was a problem hiding this comment.
This all looks good now.
Adds an additional column for pages.