Add UI for retagging content #9834
Conversation
ChrisBAshton
force-pushed
the
ui-for-retagging
branch
10 times, most recently
from
7c8803b to
e4bfeef
Compare
|
|
||
| if updater.errors.any? | ||
| sanitized_errors = updater.errors.map { |err| sanitize(err) } | ||
| flash[:alert] = "Errors with CSV input: <br>#{sanitized_errors.join('<br>')}".html_safe |
There was a problem hiding this comment.
It's a bit of shame that Rails leaves you on your own for formatting and rendering errors that aren't from an Active Model
| @@ -48,12 +68,14 @@ def find_document(row) | |||
| statistics_announcements = StatisticsAnnouncement.where(slug:) | |||
There was a problem hiding this comment.
This is a bit dubious, but I don't blame you for not touching it 😅
| end | ||
| end | ||
| status.join(", ") + ". Result: #{new_orgs.join(', ')}" | ||
| end |
There was a problem hiding this comment.
Just looking at some of the methods on this class, I do wonder if a bulk organisation retag could be an Active Model 🤔 (maybe even an Active Record model if we wanted to store the result of the operations for debugging)
I do think everything looks like a model now in Rails though, could be going overboard on it. But it might be a nice way to lean into the framework and make some of this code a bit less of a transaction script and a bit more Railsy.
There was a problem hiding this comment.
That's one I'll leave for a future PR I think - the scope of this PR is just to migrate what currently exists as a rake task, into a UI form, and not to look too closely at the current implementation 😅
No need to hard-code this markup ourselves.
- Adds route - Adds permission - Adds view - Adds link to the "More" page - Adds tests
Handle errors at the structural level (does the CSV _look_ right?) and the data integrity level (does the referenced document and organisation exist?).
- Rake task now replaced by a form - so has been removed - No need to expose `self.call` anymore - Have removed all calls to `puts` - we should be inspecting the object instead (`.errors`).
We don't need the long justification / TODO comment as Whitehall is not expected to support ordered supporting organisations. See 1253635#r1928514727
As suggested in 1253635#r1928517553, Cucumber tests are expensive and so if we can move the tests that aren't covering the happy path, into a faster, lower level area (i.e. controller tests), then that's a good idea. The dry run with errors test has been moved. The dry run and successful preview test has been removed, as that's already tested by the 'non-dry' run Cucumber test. For the test that has been removed, we've unfortunately lost one assertion: that the <textarea> contains the value we've POSTed to the controller. I tried every variation of the following, but it looks as though `@response.body` is `""` when we `render :index`, even if we add `layout: false` etc. Somewhere in our Rails test configuration we must be disabling redirected renders or something. ```ruby assert_select "textarea[name='csv_input']", csv_to_submit.strip ``` In practice, it's not a huge issue that we're missing the test, as it isn't critical functionality: publishers won't be filling in this field by hand, they'll be copying and pasting from another source.
ChrisBAshton
force-pushed
the
ui-for-retagging
branch
from
e4bfeef to
6416b94
Compare
Adds UI for "bulk retagging documents" (as it is referred to in the Support government changes docs) to change their lead and supporting organisations. This UI replaces a rake task we had to run in the past, and removes our last CSV-dependent rake task (which has been problematic since the introduction of Kubernetes).
Trello: https://trello.com/c/o9iYyNxf/623-add-ui-for-re-tagging-organisations-to-content-in-bulk
Screenshots
Link added to the "More" page:
"Retag content" form:
Form with inputs filled in:
Preview page / summary of changes:
Confirmation status:
Follow these steps if you are doing a Rails upgrade.