Finish implementing confirmation workflow #144
Conversation
matt-bernhardt
force-pushed
the
tco-101
branch
from
05c193d to
26d113c
Compare
matt-bernhardt
force-pushed
the
tco-101
branch
from
6ec0f0b to
9266ee5
Compare
matt-bernhardt
force-pushed
the
tco-101
branch
from
9266ee5 to
7e9b658
Compare
|
@matt-bernhardt the i18n check is interesting. I'll start a thread on Slack to discuss approaches. TLDR though: it's not a default enabled cop, but our config auto enables new cops even when they are |
| end | ||
|
|
||
| # The confirmation form lists options for each Category record, and then one extra option to flag the term for | ||
| # removal. "Flag" is not a category, but a separate boolean field on the Confirmation model. |
There was a problem hiding this comment.
Did you consider having Flagged for removal be a category or a boolean on a Term itself? (I know that landed in a previous commit and I'm not asking for a change, I'm just curious as something being flagged will be best to be removed from all future queries immediately (to be reviewed by us probably) so putting the flag on Term would make it easy to suppress it from all user facing views via a scope. It could be done also with the flag here, but that requires additional more complex queries any time a Term is being looked up. Not a big deal either way, I'm mostly just curious what was considered when making this decision.
I'm nervous to actually delete flagged queries every because by keeping them but suppressing them allows us to always hide them from users whereas deleting them would just allow them to resurface again if someone were ever to search for that term again. I guess our review of suppressed queries might have some thought into how likely it would be to recur and need to be re-suppressed again. Hmmm.
There was a problem hiding this comment.
I remember discussing a few options, but I'm not sure I could find where now. I do remember thinking about whether we needed 3, 4, or 5 categories - and settling on 4 with the addition of Undefined, which humans can use but the knowledge graph does not. A Flagged category didn't seem to make sense, based on the assumption that we would actually be deleting terms. If we don't actually delete terms, then perhaps that might make sense to revisit.
My thought about suppressing versus deleting terms is that we shouldn't store records we shouldn't have - keeping them in the database invites problem if the application leaks data or is breached. If a term gets re-submitted, we'll need to delete it again, but I'm not sure it's worth keeping things around in case a duplicate ever gets submitted (remembering how rare repeated searches are in the first place).
I do remember thinking about a flag on the Term, and decided against that approach because having the flag on the Confirmation preserves information about who flagged it - in case follow-up is needed about why they flagged it.
There was a problem hiding this comment.
Yeah this is complicated. There aren't many reasons we'd want to flag/delete a term and if we hit one I'm nervous to not have a way to prevent it from being shown to all future validators, keep it out of any data exports, etc. I understand your concern about wanting to delete data we shouldn't have, but I'm waffly on whether it's better to know we shouldn't have it (by flagging it as suppressed at the Term level) to make sure it's never included in our work versus having it continue to show up again in the future. I can probably be brought around to your view based on the infrequency of repeat terms, but by deleting things -- contrary to how it may feel -- we'll never actually know for certain we are preventing it from being seen from anyone (other than us as system maintainers who have raw db access). Hmm.
matt-bernhardt
force-pushed
the
tco-101
branch
from
7e9b658 to
bd39c4f
Compare
|
Okay, @JPrevost - I've pushed commits that resolve many of your concerns, and extends the (already-existing, it turns out) exclusion of the i18n cop to include the Confirmation controller. That resolves the issue for CodeClimate. Remaining is the question of how to handle flagged Terms. My thoughts on that, at the moment, are these:
What are your thoughts at this point? |
|
@matt-bernhardt this may be something best discussed synchronously, but I'll try to share my thoughts here. I think the best place to flag a Term we are concerned about is in the Term model itself. I don't think using a Category of "Flagged for review" or a boolean on a confirmation provides the affordances we'll want to suppress the Term from being used until we have reviewed and possibly chosen to delete it. A future scope on Term "without_flagged" (other names might be "displayable" or something that makes it clear what the intent is) could be used consistently throughout the application to prevent anyone from seeing flagged results until such time as we as data maintainers either remove the flag as erroneous or delete the Term. Using a Category or boolean flag on a Confirmation could also be used to create a scope, but it feels much more complicated to reason about as any time we use a Term the scope would need to look to other tables to determine if it is displayable. As we'll have so few flagged Terms, this seems like it's more work with minimal gain. With all that said:
|
|
I can see a solid rationale for adopting a scope on the Term model for "records that have not been flagged" - and that we would soon want to enforce that scope in nearly every interaction where the application asks for a list of Terms. This also feels like it calls for an interface to show which records have been hit by this flag, although the interface doesn't feel like it's in scope of the current ticket. I'm less clear whether the creation of this scope would be part of this ticket, although the conversion of all existing references to use the scope feels like it would be future work. What about something like this as a middle ground, at least for the short term but possibly for longer:
This feels like it would achieve both of our goals:
Both the record of the user's activity, and the operation needs of the application, are preserved - at the cost of a slight duplication of information. However, this duplication will come in handy if we ever decide that the flag is not justified, because we can release a term for processing while still having some breadcrumb that it was once flagged (just in case that's ever a question). I'd be comfortable with this as a route forward, and am willing to refactor the current PR toward this goal. There would then be future tickets to build the scope, leverage it across the application, and build an interface to show admins (only) what terms have been flagged. |
|
@matt-bernhardt Yup. That makes a ton of sense to have both the Category and Term boolean for the reasons you stated. Thanks for talking this through to help figure out what made sense to build towards future features versus what the future features are that we can ticket but not implement at this time. |
|
I'm thinking that rebasing this branch past the recent merges will be fine, as long as I keep the current three commits unchanged - but I want to confirm this before force-pushing. |
|
@matt-bernhardt yes that will be fine. Leave the commits, but rebase as need will work for me |
matt-bernhardt
force-pushed
the
tco-101
branch
from
5614225 to
5dd9679
Compare
** Why are these changes being introduced: A previous PR started the implementation of our confirmation workflow, but did not include a form in the view template, nor the controller logic to receive the form submissions. ** Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/tco-101 ** How does this address that need: This finishes the job of the previous PR, completing the confirmation workflow. More specifically, it: * Updates the routes information, moving the "index" display to a better route name, and defining Confirmations as resources under Terms. This makes dealing with routing more logical, and takes advantage of Rails' magic. Only the new and create methods are implemented. * Moves the confirmation form out of the Terms controller, and into a new Confirmation controller, which is where records like this should be managed, in the Confirmation#new method. * Form submissions are received in Confirmation#create, which is helped by two new private methods for dealing with category values (which are either a category_id or a "flag" boolean), and for user feedback based on the create result. * The site_nav and tests are updated to reflect the new controller and route information * The view templates are updated, most notably the new confirmation template, which now has a working form using Rails' built-in form tool. ** Document any side effects to this change: The confirmation form has some inline styles to quickly get the UI into something not-awful. Rubocop is complaining about lack of translation in the feedback messages in the Confirmation controller, The final else clause in the Confirmation#feedback_for method is not something I can figure out a test for - but I'm loathe to not have a default clause in that conditional. The .save method can return false, but usually that means an error message, which I've provided a catch for the most likely scenario (and a test showing how it gets produced). I'm not sure about one of the confirmation controller tests - the one confirming that terms disappear from the list after a confirmation is created. We already have a test for the scope that provides this list, but it felt appropriate to try and test at the controller level as well? I don't remember writing tests like this in the past, though - so maybe it isn't relevant here.
* Method comments now start with the method name, not "This method" * feedback_for comment rewritten to avoid first-person statement * Sentry messages generated in two relevant blocks (rescue method and the else block of a conditional)
The biggest change here is that we are changing the data model, moving the flag field from the Confirmation record to the Term record. This clears the way for Confirmation to treat Category as a required field, which explains the three migrations in this commit. Because "flagged" is now a category in Confirmation terms, we add that to the seeds file. Along with the data model change, we update the new confirmation form to more explicitly label fields as ID fields, in addition to no longer having a separate list entry for flagging terms. Through all of the above, the controller shifts a bit, as set_category is no longer needed to manage received values. In its place, there is now a flag_term method for setting the new flag on the related term - as long as the confirmation saves correctly. There is also a guard clause for when the confirmation does not save correctly, which means the feedback_for method gets a little simpler. --- Next up is a slight refator to adopt a strong parameters approach, which will have some additional changes with it.
This changes the confirmation controller to adopt a strong parameters posture, which includes explicitly adding the term ID and user ID fields to the submitted form (rather than handling them implicitly from other sources). During testing we also abandon the guard clause and .create methods in the controller, in favor of .new and a more explicit if/else block on the .save method. During testing, this proved necessary to confirm that the submitted record is actually saved, as the single .create step I was trying failed silently with no feedback. The "submitting a confirmation form without all fields shows an error" test is a reflection of this difficulty, confirming that the application does not quietly swallow anything if a form goes awry. Ditto with the assertions that look for confirmation counts pre- and post- action.
matt-bernhardt
force-pushed
the
tco-101
branch
from
5dd9679 to
5e44ac6
Compare
JPrevost
left a comment
JPrevost
left a comment
There was a problem hiding this comment.
Looks good. Two minor suggested documentation updates (one to remove/move a line that is no longer accurate should definitely be fixed, the other is a suggestion.
|
|
||
| # feedback_for takes the result of the confirmation.save directive above and sets an appropriate flash message. | ||
| # | ||
| # The final else clause is likely to be difficult to provoke, so we are sending a Sentry message in that block in |
There was a problem hiding this comment.
I think this line of documentation should be removed or moved as it does not reflect the current state of the method it is documenting.
There was a problem hiding this comment.
Good catch - I've rewritten this entire comment block. Thanks!
| t.save | ||
| end | ||
|
|
||
| # confirmation_flag? compares the submitted category (coerced to an integer) to the ID value for the "flagged" category. We |
There was a problem hiding this comment.
We do this at least twice in this controller. makes me nervous as it would be easy to miss updating this type of statement when the logic in the controller changes.
| # create receives the submission from the new confirmation form, creating the needed record with the help of various | ||
| # private methods. | ||
| def create | ||
| new_record = Confirmation.new(confirmation_params) |
There was a problem hiding this comment.
Strong params cleaned this up nicely. Thanks for going through that process.
There was a problem hiding this comment.
When I saw what the final diff ended up being, I agree that it makes things much simpler. I'm not completely happy that it doesn't seem to be possible to chain .require() statements in the way I originally expected, but this ended up being a bit better than I at first feared.
This finishes implementing the confirmation workflow. Full details are in the commit message, including a somewhat lengthy discussion of some side effects and concerns.
Developer
Ticket(s)
https://mitlibraries.atlassian.net/browse/TCO-101
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