Break batch invitations creation into two steps #2350
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
|
I'm going to rebase this branch on |
chrislo
force-pushed
the
batch-invitations-two-step-flow
branch
from
cd381e3 to
ee13536
Compare
|
I've checked out this branch locally and had a play - creating batches of users works as before and the two-stage process seems to work well in the happy path case. I noticed that if you start a batch upload, upload a CSV but stop before assigning permissions, the batch upload appears in the list of batch uploads in |
|
I've addressed the issue I noticed with not being able to resume an incomplete batch in c7ea253. |
I'm planning on introducing a new "status" to BatchInvitations and I believe this helper method is where that concept currently lives. I've avoided the temptation to test every possible scenario exhaustively. Before I looked this closely, though, I'd imagined that this status and BatchInvitation#outcome mapped 1:1, and that BatchInvitation#outcome and BatchInvitationUser#outcome were related. I hope that these tests are clear enough to demonstrate that neither of those assumptions are true.
I think this change removes an order dependency between `#all_successful?` and BatchInvitation's other state predicate methods. We use `#all_successful?` as a proxy for "was the batch invitation processed successfully?" I don't think it makes sense to say that a non-completed (potentially in progress or just never started) batch invitation has been successful purely based on the fact that none of its batch invitation users have (so far) been marked as "failed".
We're planning to break BatchInvitation creation into two separate steps: 1. Upload user info 2. Manage permissions One way of making that work requires us to know whether a given batch invitation has already had permissions assigned to it or not
This change makes the use of the `#has_permissions?` and `#in_progress?` predicates independent of one another (i.e. they don't always need to be called together, in the correct order) and it protects batch invitations in a very specific state from being misidentified. We're going to break the batch invitation creation process into two parts, the second part being the selection of the batch invitation's permissions. A batch invitation won't technically be in-progress (i.e. inviting users and sending out emails) until its permissions have been selected. In theory, a user could abandon a half-finished batch invitation, leaving it created in the database but without permissions. Since it's already true now that a batch invitation isn't in-progress until its permissions have been selected, this change in logic doesn't change existing behaviour.
There's no way currently, even after we've broken the batch invitation from across two pages, that a BatchInvitation without any permissions will appear on a page that wants to describe its state. But I don't find it unlikely that this could change without notice, so I'm adding this with future us in mind
A test documenting the behaviour when a user visits the #show page for a BatchInvitation that doesn't have any permissions (it just happened to already work this way)
This is mostly copy-pasted from BatchInvitationController's existing permissions-handling code. We're en route to breaking the batch invitation creation process into a two step form and this is going to be the second step.
Instead of freeloading on the existing BatchInvitation controller action policies
This form is the second step of the batch invitation creation process. Once a batch invitation's permissions have been set, it's considered to be "in progress" and these details shouldn't be changed.
The existing batch invitations controller handles uploading the CSV of users and setting a default organisation, and it delegates managing permissions to the new batch invitation permissions controller. The BatchInvitation and its BatchInvitationUsers will still be persisted once this first form is submitted, but the jobs, emails, etc won't be kicked-off (the batch invitation won't be "in progress") until the permissions form has been submitted.
This allows a user to complete an unfinished batch (i.e. one where the user has uploaded a CSV file but not selected any permissions).
chrislo
force-pushed
the
batch-invitations-two-step-flow
branch
from
c7ea253 to
37397e3
Compare
chrislo
approved these changes
Sep 12, 2023
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.
https://trello.com/c/FzaiUjkY/127-break-batchinvitations-new-into-two-separate-steps-to-prepare-for-migrating-to-the-design-system
This is the first step of moving
batch_invitations#newto the design system. It breaks the large single page form into two separate steps across their own pages:The second step is implemented in its own controller.
Step 1.
Step 2.