Simple confirmation workflow #133
Merged
Conversation
This file contains 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
matt-bernhardt
force-pushed
the
user-categorization
branch
from
512483d to
004ecae
Compare
matt-bernhardt
changed the title
** Why are these changes being introduced: We are adding a Confirmation class for users to provide ground truth for how different Terms should be categorized. The first step is to update our documentation to work out how this is going to work. ** Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/tco-8 ** How does this address that need: This updates the class diagram and workflow documentation to include the Confirmation class. * User and Confirmation are now grouped in the class diagram, with a common visual language. * The categorization workflow document is updated to use the new vocabulary "confirmation" rather than our earlier "validation". ** Document any side effects to this change: As this commit only changes documentation, there are no side effects yet.
** Why are these changes being introduced: With the documentation updated, we need to actually change the models. ** Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/tco-8 ** How does this address that need: * Adds the Confirmation model, with relationships to User, Term, and Category (optionally). The category relationship is optional because there is also the option for a user to flag the term for removal. This flag is stored on the Confirmation model so that we have a record of who flagged it, in case we want to follow up with them. The implication is that we will review these flags, not perform the removal immediately. There is a uniqueness constraint on the Confirmation model, to limit users from confirming the same term twice. * Two scopes are added to the Term model which filter records that have (and have not) been confirmed by a user. * A fourth category is added to the database seeds - "Undefined". The automated categorization does not interact with this category, but users have the option to place a term there in their workflow. The UI which is coming in the next commit then will provide five options for manual confirmation: one for each category, and then a "flag" option. * Tests and fixtures are added to support and confirm all of the above. ** Document any side effects to this change: There are a few side effects here: * While adding the 'flagged' field makes sense to me on the Confirmation model, it may also need to be added to the Term model (or at least add a scope to grab flagged records) * The existing scopes on the Term model do not pay attention to which user has confirmed the term - so at the moment getting multiple confirmations from multiple users may not be possible without further work. * Because we are changing the database seeds, we will need to take care when promoting this change ito production.
** Why are these changes being introduced: * With the models updated, we now turn to the controller and view layer. ** Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/tco-8 ** How does this address that need: * Defines two new routes for the confirmation workflow - an index and a form. * We add the Pagy gem to the application, for use on the confirmation index. * Adds a Term controller, with two methods for an index and form view. The controller has tests for its use of the ability model. * The ability model adds the ability to manage confirmations for any authenticated user. * View templates are added for these two displays, and a link to the index is added to the navigation. The form is a stub, with no actual controls (and no controller method to process their submission). ** Document any side effects to this change: * I'm not sure whether the controller needs more tests. * Our theme gem appears to anticipate the use of Kaminari, not Pagy - not sure what the lift will be to polish the pagination into something more presentable. * As noted above, the form is faked - there is no actual input fields, nor a controller method to receive them.
matt-bernhardt
force-pushed
the
user-categorization
branch
from
004ecae to
61cc08a
Compare
16 tasks
jazairi
requested changes
Nov 8, 2024
There was a problem hiding this comment.
Thanks for your patience on this review. I think I grabbed this right before getting sick. 🙃
For a variety of reasons, it's difficult this week for me to process this with the level of care I'd normally like to have. However, I don't see any issues in the code, and the approach makes sense. The only thing I should note is that, having never used pagy, I can't really confirm whether it's set up or not. I trust you on that, though.
As I mentioned on Slack, something weird is happening with text color in the updated diagram. If you can set the text color explicitly so it looks right in light and dark mode, I think this is 
.
|
I've pushed a commit to better specify the text color in the classes diagram - thanks for catching this! |
jazairi
approved these changes
Nov 13, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This implements most of the confirmation workflow, wherein users of the application review search terms and provide some ground truth feedback about how they should ideally be categorized by the automated processes.
This PR does not fully implement this workflow, because the UI does not include a working form (and the Term controller has no method to receive that input). However, it gets close enough that the next PR should be able to do so easily. This PR is already verging on too big, so I'd like to ask for review now.
The biggest thing to note here is that this adds a fourth category to the application, for "Undefined". The intent is not for the application to make use of this option during the Categorization workflow, but humans should be given this option in order for the confirmation form to make sense.
Developer
Ticket(s)
https://mitlibraries.atlassian.net/browse/TCO-8 (overall confirmation epic)
https://mitlibraries.atlassian.net/browse/TCO-99 (UI design ticket)
https://mitlibraries.atlassian.net/browse/TCO-100 (confirm UI requirements)
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)
Please note while I've looked at Wave with this change, the UI is largely unconsidered. We should involve our UX colleagues in more thoughtfully considering this interface before putting it into production.
Documentation
ENV
Stakeholders
Dependencies and migrations
YES dependencies are updated
YES migrations are included
Reviewer
Code
added technical debt.
Documentation
(not just this pull request message).
Testing