Work in progress toward Transfer processing form #705
Conversation
matt-bernhardt
force-pushed
the
etd-277-transfer-processing-2
branch
from
e2209fc to
30b1b0a
Compare
** Why are these changes being introduced: * These two tickets (and the mockups which inspired them) call for a web form to facilitate selecting specific files in a given Transfer record. Each file will have a checkbox, allowing the processor to identify which should move next. ** Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/ETD-277 * https://mitlibraries.atlassian.net/browse/ETD-278 ** How does this address that need: * This extends the show route on the Transfer controller, building a web form. * The page shows overall information about the Transfer record (such as the submitter's name, department, and notes). * The form lists all the attached Files by name, with a checkbox for each. This form does not use SimpleForm, just the basic Rails form helpers. * There is a blank area next to the file list that will in future work be extended to provide a Thesis selection UI. * There is a files method in the controller that receives this form's parameters. The method currently does no work, only defining a flash message with the selected file IDs. That method will be extended in future tickets. * The file controller has one additional permitted parameter, for the list of file IDs. * There are some form-specific styles added to the stylesheet. * The transfer controller tests have been restructured; hopefully they are a bit clearer now. ** Document any side effects to this change: * The form template is not quite what I expected it to be, but I think it is still acceptable in the Rails world. I hope. * The test for the files method in the transfer controller is a bit kludgy, because our existing fixtures do not have file records. This probably needs to be fixed before this merges, but for now this test creates a new Transfer record which does have a file.
matt-bernhardt
force-pushed
the
etd-277-transfer-processing-2
branch
from
d551d08 to
1a64fec
Compare
| filelist.each do |file| | ||
| flash[:success] += ("File ID: " + file.to_s + "<br>").html_safe | ||
| end | ||
| flash[:success] += "This has been a test of the files method. If this were an actual method, these " + filelist.count.to_s + " files would have been transferred." |
There was a problem hiding this comment.
I know this is placeholder text, but since we've been calling "Transfers" the process of a Department admin adding files to our system lets refer to moving files from those Transfers to Theses not "transferred" to avoid confusion. I think something like "attached to the selected thesis" might be clearer?
| @@ -45,6 +45,7 @@ | |||
| } | |||
|
|
|||
| get 'transfer/confirm', to: 'transfer#confirm', as: 'transfer_confirm' | |||
| post 'transfer/files', to: 'transfer#files', as: 'transfer_files' | |||
There was a problem hiding this comment.
We discussed on slack possibly moving this to the Thesis controller as the action is on Theses and not Transfers (not blocking for this work, but wanted to note it here for future us). We'll likely address this in the following PRs as we haven't made a decision yet.
| end | ||
|
|
||
| test 'basic user cannot submit or view a transfer' do | ||
| # ~~~~~~~~~~~~~~~~~~~~~~~~~~ new transfer form ~~~~~~~~~~~~~~~~~~~~~ |
There was a problem hiding this comment.
This series of tests is named "can / cannot submit a transfer" but really is about accessing the transfer new form and doesn't test submitting. I think ideally we'd have a series of tests for the form and a series of tests for the POST itself.
An existing test test 'redirect after successful submission' could be the pattern for each user type (I think).
As we didn't have that before, it is technically out of scope for this work so maybe we can add a maintenance ticket?
There was a problem hiding this comment.
Looking at this comment and the tests, it would have been equivalent effort to create the ticket and just update the tests - so I'm about to push a commit that should address this directly. If it doesn't, I'll create the ticket.
|
I've pushed two new commits that address your first and third comments (the second required no action yet) - so if you get a chance and can look at those, let me know if they resolve your concerns? |
This adds the first part of the transfer processing form. Along the way it also changes the ability model to restrict who can see this path, and it restructures the controller tests for Transfer records.
This work addresses two tickets:
https://mitlibraries.atlassian.net/browse/ETD-277
https://mitlibraries.atlassian.net/browse/ETD-278
Special note: I'm not thrilled by the approach taken by the last test in the controller test file, but I don't see an alternative other than re-doing the fixtures (which feels like work that's going to spiral). If that's needed, I'll do it - but I wanted to make that decision via code review.
Developer
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)
Code Reviewer
(not just this pull request message)
Requires database migrations?
NO
Includes new or updated dependencies?
NO