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

Only display unassigned files by default #740

Merged
merged 1 commit into from
Jul 15, 2021
Merged

Conversation

JPrevost
Copy link
Member

@JPrevost JPrevost commented Jul 14, 2021

Why are these changes being introduced:

  • The list of files can be quite large in some cases
  • Scrolling through files that have already been processed can be
    annoying and inefficeint
  • Most of the time, it is not useful to see the already assigned files

Relevant ticket(s):

How does this address that need:

  • Enables conditional display of the already assigned files during
    the Transfer processing workflow
  • The default is to not display assigned files

Document any side effects to this change:

  • Choosing to display assigned files should be sticking throughout the
    processing of that Transfer but would need to be reapplied when
    changing to a different Transfer. Being sticky beyond what we have
    implemented here would require using a session or local storage which
    didn't seem necessary at this time but wouldn't be difficult to
    implemente in the future if that is the desired behavior.
  • The display of the <li> elements had to be adjusted slightly to
    prevent blank <li>s being displayed for the files we are skipping.

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-740 July 14, 2021 19:28 Inactive
@coveralls
Copy link

coveralls commented Jul 14, 2021

Coverage Status

Coverage remained the same at 94.755% when pulling f2f976d on etd-366-shrinky-list into 25e8948 on main.

@JPrevost JPrevost requested a review from matt-bernhardt July 15, 2021 13:33
@JPrevost JPrevost marked this pull request as ready for review July 15, 2021 13:35
@JPrevost JPrevost removed the request for review from matt-bernhardt July 15, 2021 13:36
@matt-bernhardt matt-bernhardt self-assigned this Jul 15, 2021
Why are these changes being introduced:

* The list of files can be quite large in some cases
* Scrolling through files that have already been processed can be
  annoying and inefficeint
* Most of the time, it is not useful to see the already assigned files

Relevant ticket(s):

* https://mitlibraries.atlassian.net/browse/ETD-366

How does this address that need:

* Enables conditional display of the already assigned files during
  the Transfer processing workflow
* The default is to not display assigned files

Document any side effects to this change:

* Choosing to display assigned files should be sticking throughout the
  processing of that Transfer but would need to be reapplied when
  changing to a different Transfer. Being sticky beyond what we have
  implemented here would require using a session or local storage which
  didn't seem necessary at this time but wouldn't be difficult to
  implemente in the future if that is the desired behavior.
* The display of the <li> elements had to be adjusted slightly to
  prevent blank <li>s being displayed for the files we are skipping.
@JPrevost JPrevost force-pushed the etd-366-shrinky-list branch from 1ea2765 to f2f976d Compare July 15, 2021 13:43
@JPrevost JPrevost temporarily deployed to thesis-submit-pr-740 July 15, 2021 13:43 Inactive
@JPrevost JPrevost requested a review from matt-bernhardt July 15, 2021 13:43
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.

Huh... coming into this ticket I was expecting it to go one of two ways (filter the list via javascript without a page load, or split the record loading in the controller). This is going down a third path, but I think I see the motivation. Interesting. This both allows for testing of the feature (harder with a javascript implementation), and doesn't grow the controller as much as I thought it might (with multiple record loading paths).

The tests pass, I see the feature working, so :shipit:

@JPrevost JPrevost merged commit 2a91029 into main Jul 15, 2021
@JPrevost JPrevost deleted the etd-366-shrinky-list branch July 15, 2021 15:18
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