AO3-5748 Split off assignments with both offer and pinch hitter#5555
AO3-5748 Split off assignments with both offer and pinch hitter#5555neuroalien wants to merge 1 commit intootwcode:masterfrom
Conversation
Usually, when a mod assigns a pinch-hitter through other means, a brand new ChallengeAssignment is created with a pinch_hitter_id (rather than adding a pinch_hitter_id to the existing ChallengeAssignment). Let's do the same for the case of a pinch hitter being assigned at the same time as the offer signup. We've chosen to do this at the point of saving the current configuration, so we can show the resulting assignments. We hope the maintainer won't double-assign again, thus causing a chain of splits. I've chosen to do this in a separate method, invoked after `update_placeholder_assignments!` has done its thing, because otherwise our freshly created assignment would have been deleted as being a "leftover placeholder" by the "# if this signup has at least one giver now, get rid of any leftover placeholders" check. I didn't want to mess with how leftover placeholders are defined, since we don't have any open issues about them.
neuroalien
force-pushed
the
AO3-5748-double-assignment
branch
from
18ef4f0 to
45f1f0f
Compare
| end | ||
|
|
||
| ChallengeAssignment.update_placeholder_assignments!(@collection) | ||
| ChallengeAssignment.split_off_write_in_giver_assignments!(@collection) |
There was a problem hiding this comment.
I'm wondering if this should go inside update_placeholder_assignments!, since at the moment it seems like we'll always want to do them both at the same time?
| def double_assigned? | ||
| pinch_hitter && offer_signup && pinch_hitter.user != offering_user | ||
| end | ||
|
|
||
| def split |
There was a problem hiding this comment.
Could you add tests for these methods as well? (I'm also wondering if it's worth making them private, which would mean harder to test but a smaller public interface for this class 🤔 )
| put :set, params: params | ||
|
|
||
| assignment.reload | ||
| expect(assignment.pinch_hitter).to be_nil |
There was a problem hiding this comment.
Could you add an assertion to make sure the offer signup is not cleared here? (Probably paranoid of me, but just in case 😅 )
| new_assignment.save! | ||
|
|
||
| self.pinch_hitter = nil | ||
| save! |
There was a problem hiding this comment.
Thinking about these 2 saves, I'm leaning towards wrapping it in an explicit transaction. That way, we don't "lose" signup information if there's a problem with one of these writes. But it means we could still hit the case mentioned in the bug iff there's some database issue between the start and end of the assignments change. What do you think?
3 participants
Split off assignments with both offer and pinch hitter
Pull Request Checklist
as the first thing in your pull request title (e.g.
AO3-1234 Fix thing)until they are reviewed and merged before creating new pull requests.
Issue
https://otwarchive.atlassian.net/browse/AO3-5748
Purpose
What does this PR do?
Splits off assignments that have both an offer signup and a pinch hitter, into two separate assignments, one owned by the offering pseud, the other owned by the pinch hitter, neither leaking data about the other.
Usually, when a mod assigns a pinch-hitter through other means, a brand new ChallengeAssignment is created with a pinch_hitter_id (rather than adding a pinch_hitter_id to the existing ChallengeAssignment).
Let's do the same for the case of a pinch hitter being assigned at the same time as the offer signup.
We've chosen to do this at the point of saving the current configuration, so we can show the resulting assignments. We hope the maintainer won't double-assign again, thus causing a chain of splits.
The result, for a three-user challenge, will look something like this:
I've chosen to do this in a separate method, invoked after
update_placeholder_assignments!has done its thing, because otherwise our freshly created assignment would have been deleted as being a "leftover placeholder" by the "# if this signup has at least one giver now, get rid of any leftover placeholders" check. I didn't want to mess with how leftover placeholders are defined, since we don't have any open issues about them. "If it ain't broke" etc.Testing Instructions
How can the Archive's QA team verify that this is working as you intended?
The JIRA issue has detailed instructions.
References
Are there other relevant issues/pull requests/mailing list discussions? If not, you can remove this section.
Yes, this should also fix https://otwarchive.atlassian.net/browse/AO3-5752.
Credit
What name and pronouns should we use to credit you in the Archive of Our Own's Release Notes?
alien, they/them