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

Bugfix for broken Hold dashboard searches #720

Merged
merged 1 commit into from
Jun 11, 2021

Conversation

jazairi
Copy link
Contributor

@jazairi jazairi commented Jun 10, 2021

Why these changes are being introduced:

Certain fields in the Hold dashboard, which are generated via convenience
methods and not stored in the db, were defined as Field::String. Because
Field::String is searchable by default, this caused the dashboard search
to break when it tried to query nonexistent columns.

Relevant ticket(s):

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

How this addresses that need:

This changes the offending fields to Field::Text, which are not
searchable by default.

Side effects of this change:

  • When I fixed this bug, I noticed that the only searchable field was
    status. I added thesis title as another searchable field, since there
    aren't many Hold model fields that are searchable in administrate.
  • Notably, status is searchable only if you add a colon to the
    end of the querystring (e.g., 'active:'), due to some weirdness with
    how collection filters work in administrate.

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-720 June 10, 2021 19:03 Inactive
@coveralls
Copy link

coveralls commented Jun 10, 2021

Coverage Status

Coverage increased (+0.01%) to 94.559% when pulling 5492db6 on etd-304-holds-author-names-bugfix into c6b5562 on main.

@jazairi jazairi force-pushed the etd-304-holds-author-names-bugfix branch from 21666e9 to a0119a7 Compare June 11, 2021 13:57
@jazairi jazairi temporarily deployed to thesis-submit-pr-720 June 11, 2021 13:57 Inactive
@jazairi
Copy link
Contributor Author

jazairi commented Jun 11, 2021

Hm...locally I'm seeing 100% coverage for the Hold dashboard. Not sure why coveralls is reporting something different.

@matt-bernhardt matt-bernhardt self-assigned this Jun 11, 2021
Copy link
Member

@matt-bernhardt matt-bernhardt left a comment

Choose a reason for hiding this comment

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

Well done!

I'm also not sure why coverage isn't noted for those last two lines in the filter - it seems that they should be counted. When I run the tests locally I'm getting 95.39% coverage, rather than the 94.32% reported here.

:shipit:

Why these changes are being introduced:

Certain fields in the Hold dashboard, which are generated via convenience
methods and not stored in the db,  were defined as Field::String. Because
Field::String is searchable by default, this caused the dashboard search
to break when it tried to query nonexistent columns.

Relevant ticket(s):

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

How this addresses that need:

This changes the offending fields to Field::Text, which are not
searchable by default.

Side effects of this change:

* When I fixed this bug, I noticed that the only searchable field was
status. I added thesis title as another searchable field, since there
aren't many Hold model fields that are searchable in administrate.
* Notably, status is searchable only if you add a colon to the
end of the querystring (e.g., 'active:'), due to some weirdness with
how collection filters work in administrate.
@jazairi jazairi force-pushed the etd-304-holds-author-names-bugfix branch from a0119a7 to 5492db6 Compare June 11, 2021 18:15
@jazairi jazairi temporarily deployed to thesis-submit-pr-720 June 11, 2021 18:15 Inactive
@jazairi
Copy link
Contributor Author

jazairi commented Jun 11, 2021

Thanks, @matt-bernhardt! I'm seeing 95.39% coverage in the latest run, so I think that coverage report ran before I wrote tests for this. In any case, I think we're good to merge.

@jazairi jazairi merged commit de6961b into main Jun 11, 2021
@jazairi jazairi deleted the etd-304-holds-author-names-bugfix branch June 11, 2021 18:19
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