Add option to confirm only categorized terms#174
Conversation
** Why are these changes being introduced: The confirmation workflow was predicated on humans providing a second opinion on categorization, allowing us to review how the application is already performing. However, when we initially rolled out this workflow, it allowed humans to provide confirmation on every term in the system, and not just those which the system had categorized. ** Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/tco-126 ** How does this address that need: This adds: * A categorized scope to the Term model * Forks the logic in the term controller's "unconfirmed" method, returning two different recordsets. By default it includes only categorized terms (via the scope added above). However, there is a URL parameter to leave this restriction out. * The template for this list of terms has buttons to switch between the two modes. The default behavior applies the categorized scope, while we can still see all terms via the other button. * We add tests for both the model scope and the branching logic in the controller, making sure that they behave as expected. ** Document any side effects to this change: * I'm a little concerned that I've missed writing tests in some cases, but I think this covers most of the feature.
matt-bernhardt
force-pushed
the
tco-126-confirmation-scope
branch
from
6239436 to
9dc1a31
Compare
JPrevost
left a comment
JPrevost
left a comment
There was a problem hiding this comment.
I'd like to see the UI affordance of selecting between "verification" mode and "categorization" mode removed from default logged in users. If you feel strongly that everyone should get this affordance, I'd be open to listening as to why and revise my review to approved.
| # This provides the list of terms that are awaiting confirmation. | ||
| # This provides the list of terms that are awaiting confirmation. By default this shows only terms which have been | ||
| # categorized automatically. Adding `type=all` to the querystring will show _all_ terms which the user has not yet | ||
| # confirmed. |
There was a problem hiding this comment.
We don't need to change for this PR as the default is what we expect people to use for the foreseeable future, but it might be better longterm to make this a sticky option by setting it as a session variable or cookie. I don't even think we need a ticket for that as we may never come back to it, but just wanted to share that the ergonomics of this is a bit awkward if someone is trying to "categorize" terms rather than validate categorizations.
There was a problem hiding this comment.
Yeah, having the choice be sticky seems like something that could be useful in the future, or even a workflow that just picks terms according to some logic, so that the user never goes back to a list-of-terms page.
Regardless, I agree that this is a concern for the future should we ever really start using this workflow.
| <p>The terms listed here have not yet been reviewed by a person and placed into a category. Clicking on any term will | ||
| take you to the form for submitting this information.</p> | ||
|
|
||
| <p class="wrap-filters">View: |
There was a problem hiding this comment.
I'm not sure if this UI option should be displayed to all users. I believe our goal is to just have people validate for the foreseeable future and this may make that slightly more confusing.
I'd recommend either hiding this behind admin capabilities or removing it entirely. Having the feature available I like...presenting the option to all logged in users feels like we are possibly inviting trouble.
There was a problem hiding this comment.
I've updated the implementation to enact a restriction like this, so basic users don't see (and can't use) the broader scope. I've also written a number of new controller tests to confirm that things behave the way I expect. I'm not particularly happy with having a test that's this tightly coupled to UI text, but I'd rather this than not have a test for it at all.
test/models/term_test.rb
Outdated
| Categorization.create!(new_record) | ||
|
|
||
| # The term has gained a category, but the categorized scope has not changed size. | ||
| assert_operator term_category_count, :<, t.categorizations.count |
There was a problem hiding this comment.
No change necessary, but term_category_count as a variable name made me have to think more than felt necessary as to what is going on here. Maybe something like orig_category_count as it gets compared to the same count after a categorization is made might be better?
There was a problem hiding this comment.
Yeah, I can see how a different name might be better. I've tried to make it something clearer, and I particularly like the orig_ prefix.
- The name of a variable in the Term model test suite has been updated to hopefully be clearer about its use. - The confirmation workflow is now restrictd to already-categorized terms for non-admin users. Admins will still have an ability to load all terms for this workflow, but as noted this is not the smoothest workflow yet. Side note: the restriction being enacted here is on the ability to load the _index_ in a specific state, but the actual confirmation form is not affected. A non-admin user who understands the URL structure will be able to guess the right URL for a given term (or pick one randomly), and submit a confirmation for a term that has not been categorized. I believe this is an acceptable risk, as the impact of such a user is that we'd have some extra confirmation records in the system - which are useful only for adjusting future operations, so I struggle to think of a downside to having some extras.
3 participants
Developer
Why are these changes being introduced:
The confirmation workflow was predicated on humans providing a second opinion on categorization, allowing us to review how the application is already performing. However, when we initially rolled out this workflow, it allowed humans to provide confirmation on every term in the system, and not just those which the system had categorized.
Relevant ticket(s):
https://mitlibraries.atlassian.net/browse/tco-126
How does this address that need:
This adds:
A categorized scope to the Term model
Forks the logic in the term controller's "unconfirmed" method, returning two different recordsets. By default it includes only categorized terms (via the scope added above). However, there is a URL parameter to leave this restriction out.
The template for this list of terms has buttons to switch between the two modes. The default behavior applies the categorized scope, while we can still see all terms via the other button.
We add tests for both the model scope and the branching logic in the controller, making sure that they behave as expected.
Document any side effects to this change:
Accessibility
all issues introduced by these changes have been resolved or opened
as new issues (link to those issues in the Pull Request details above)
Documentation
ENV
Stakeholders
Dependencies and migrations
NO dependencies are updated
NO migrations are included
Reviewer
Code
added technical debt.
Documentation
(not just this pull request message).
Testing