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.

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

Add holds by source report #919

Merged
merged 1 commit into from
Mar 4, 2022
Merged

Add holds by source report #919

merged 1 commit into from
Mar 4, 2022

Conversation

jazairi
Copy link
Contributor

@jazairi jazairi commented Feb 25, 2022

Why these changes are being introduced:

We need to be able to see the holds for a given degree period,
filtered by hold source.

Relevant ticket(s):

https://mitlibraries.atlassian.net/browse/ETD-462

How this addresses that need:

This adds a report of holds that allows filtering on thesis
grad date and hold source.

Side effects of this change:

  • Report#extract_terms now checks for the type of items in the
    collection, so as to accommodate non-thesis collections.

Developer

  • All new ENV is documented in README
  • All new ENV has been added to Heroku Pipeline, Staging and Prod
  • ANDI or Wave has been run in accordance to
    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)
  • Stakeholder approval has been confirmed (or is not needed)

Code Reviewer

  • The commit message is clear and follows our guidelines
    (not just this pull request message)
  • There are appropriate tests covering any new functionality
  • The documentation has been updated or is unnecessary
  • The changes have been verified
  • New dependencies are appropriate or there were no changes

Requires database migrations?

NO

Includes new or updated dependencies?

NO

@mitlib mitlib temporarily deployed to thesis-submit-pr-919 February 25, 2022 23:04 Inactive
@jazairi jazairi force-pushed the etd-462-holds-by-source branch from ffc39d3 to 33ea315 Compare February 25, 2022 23:08
@jazairi jazairi temporarily deployed to thesis-submit-pr-919 February 25, 2022 23:08 Inactive
@coveralls
Copy link

coveralls commented Feb 25, 2022

Coverage Status

Coverage increased (+0.03%) to 97.123% when pulling 99b7508 on etd-462-holds-by-source into 85b0186 on main.

@jazairi
Copy link
Contributor Author

jazairi commented Feb 25, 2022

It occurred to me a little late in writing this report that it might actually belong in the Hold model and controller instead of Report, since it doesn't use many of the Report features. I'm ambivalent as to whether it's worth a refactor, but I wanted to put that on the radar of whoever reviews this.

Copy link
Member

@JPrevost JPrevost left a comment

Choose a reason for hiding this comment

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

There are some N+1 queries in this report. See note in other PR for how to detect and please let me know if you'd like to pair or further discuss how to resolve.

Additionally, as a non-blocking comment it's a bit odd that the link to the Thesis from the Hold list goes to an Edit view and not either the Thesis#show or Hold#show. Is this hard coded in the Report framework? (I tried to figure out how the links got generated and gave up).

@jazairi
Copy link
Contributor Author

jazairi commented Mar 1, 2022

@JPrevost We'd initially linked to the show view in reports, but stakeholders requested the edit view instead. More info here: db49c1c

I'll look into fixing the N+1 queries shortly. Thanks!

@JPrevost JPrevost temporarily deployed to thesis-submit-pr-919 March 2, 2022 21:16 Inactive
@jazairi jazairi temporarily deployed to thesis-submit-pr-919 March 3, 2022 15:53 Inactive
@jazairi jazairi requested a review from JPrevost March 3, 2022 15:55
Why these changes are being introduced:

We need to be able to see the holds for a given degree period,
filtered by hold source.

Relevant ticket(s):

https://mitlibraries.atlassian.net/browse/ETD-462

How this addresses that need:

This adds a report of holds that allows filtering on thesis
grad date and hold source.

Side effects of this change:

* Report#extract_terms now checks for the type of items in the
collection, so as to accommodate non-thesis collections.
* Performance may be slow due to possible N+1 queries. We do
not have the test data to confirm this, but we will make changes
as needed depending on its behavior in production.

Co-Authored-By: Jeremy Prevost <jprevost@mit.edu>
@jazairi jazairi force-pushed the etd-462-holds-by-source branch from 29ee59c to 99b7508 Compare March 4, 2022 17:01
@jazairi jazairi temporarily deployed to thesis-submit-pr-919 March 4, 2022 17:02 Inactive
@jazairi jazairi merged commit aa2c569 into main Mar 4, 2022
@jazairi jazairi deleted the etd-462-holds-by-source branch March 29, 2022 21:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants