Fix bug in consented_to_proquest scope ignoring null values#1108
Merged
Fix bug in consented_to_proquest scope ignoring null values#1108
Conversation
Why these changes are being introduced: The consented_to_proquest scope returns thesis for which all authors have a proquest_allowed value of true and not false. This ignores author records with a null proquest_allowed value, so if a thesis has multiple authors and at least one of them has not selected an opt-in status, it will incorrectly be flagged as ready for ProQuest export. Relevant ticket(s): https://mitlibraries.atlassian.net/browse/ETD-588 https://mitlibraries.atlassian.net/browse/ETD-603 How this addresses that need: This chains an additional `excluding` call to the scope that checks for authors with null proquest_allowed, and adds regression tests to confirm the correct behavior. Side effects of this change: The additional `excluding` call is a bit ugly, but after fiddling with it for a while I decided it was more important to land a working solution than an elegant one.
jazairi
force-pushed
the
etd-588-bugfix-for-conflict-status
branch
from
5575951 to
044b7f5
Compare
JPrevost
approved these changes
Jan 18, 2023
jazairi
added a commit
that referenced
this pull request
Jan 19, 2023
Why these changes are being introduced: This is a similar issue to the one solved in #1108, but in the not_consented_to_proquest scope. Here, the scope excludes all theses with an author ProQuest consent value of 'true'. This works in most cases, except when one author has opted in and another hasn't, that thesis should still be partially harvested. With the current logic, it will not show up in the export preview because the scope sees a 'true' consent value and thus excludes the thesis. Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/ETD-603 How this addresses that need: This inverts the logic of the not_consented_to_proquest scope. Instead of excluding theses with at least one ProQuest consent value of true, it includes theses with at least one ProQuest consent value of false or nil, using the 'or' method to return the union. Side effects of this change: Since 'includes' is not compatible with the 'or' method, the scope now uses 'left_joins' to load the author records. This also means that the the result will be an ActiveRecord assocation rather than an array. ActiveJob does not accept associations as params, so we are now casting the params as arrays. I've done with this with both the full and partial export sets to avoid confusion, even though it's only needed at this stage for the partial export set.
jazairi
added a commit
that referenced
this pull request
Jan 19, 2023
Why these changes are being introduced: This is a similar issue to the one solved in #1108, but in the not_consented_to_proquest scope. Here, the scope excludes all theses with an author ProQuest consent value of 'true'. This works in most cases, except when one author has opted in and another hasn't, that thesis should still be partially harvested. With the current logic, it will not show up in the export preview because the scope sees a 'true' consent value and thus excludes the thesis. Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/ETD-603 How this addresses that need: This inverts the logic of the not_consented_to_proquest scope. Instead of excluding theses with at least one ProQuest consent value of true, it includes theses with at least one ProQuest consent value of false or nil, using the 'or' method to return the union. Side effects of this change: Since 'includes' is not compatible with the 'or' method, the scope now uses 'left_joins' to load the author records. This also means that the the result will be an ActiveRecord assocation rather than an array. ActiveJob does not accept associations as params, so we are now casting the params as arrays. I've done with this with both the full and partial export sets to avoid confusion, even though it's only needed at this stage for the partial export set.
9 tasks
jazairi
added a commit
that referenced
this pull request
Jan 19, 2023
Why these changes are being introduced: This is a similar issue to the one solved in #1108, but in the not_consented_to_proquest scope. Here, the scope excludes all theses with an author ProQuest consent value of 'true'. This works in most cases, except when one author has opted in and another hasn't, that thesis should still be partially harvested. With the current logic, it will not show up in the export preview because the scope sees a 'true' consent value and thus excludes the thesis. Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/ETD-603 How this addresses that need: This inverts the logic of the not_consented_to_proquest scope. Instead of excluding theses with at least one ProQuest consent value of true, it includes theses with at least one ProQuest consent value of false or nil, using the 'or' method to return the union. Side effects of this change: Since 'includes' is not compatible with the 'or' method, the scope now uses 'left_joins' to load the author records. This also means that the the result will be an ActiveRecord assocation rather than an array. ActiveJob does not accept associations as params, so we are now casting the params as arrays. I've done with this with both the full and partial export sets to avoid confusion, even though it's only needed at this stage for the partial export set.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why these changes are being introduced:
The consented_to_proquest scope returns thesis for which all authors have a proquest_allowed value of true and not false. This ignores author records with a null proquest_allowed value, so if a thesis has multiple authors and at least one of them has not selected an opt-in status, it will incorrectly be flagged as ready for ProQuest export.
Relevant ticket(s):
https://mitlibraries.atlassian.net/browse/ETD-588
https://mitlibraries.atlassian.net/browse/ETD-603
How this addresses that need:
This chains an additional
excludingcall to the scope that checks for authors with null proquest_allowed, and adds regression tests to confirm the correct behavior.Side effects of this change:
The additional
excludingcall is a bit ugly, but after fiddling with it for a while I decided it was more important to land a working solution than an elegant one.Developer
our guide and
all issues introduced by these changes have been resolved or opened as new
issues (link to those issues in the Pull Request details above)
Code Reviewer
(not just this pull request message)
Requires database migrations?
NO
Includes new or updated dependencies?
NO