Skip to content

AO3-5748 Split off assignments with both offer and pinch hitter #5555

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
neuroalien wants to merge 1 commit into
base: master
Choose a base branch
from
Open

AO3-5748 Split off assignments with both offer and pinch hitter #5555

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions app/controllers/challenge_assignments_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,7 @@ def set
end

ChallengeAssignment.update_placeholder_assignments!(@collection)
ChallengeAssignment.split_off_write_in_giver_assignments!(@collection)
Copy link
Member

@brianjaustin brianjaustin Jan 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

if @assignments.empty?
flash[:notice] = "Assignments updated"
redirect_to collection_potential_matches_path(@collection)
Expand Down
20 changes: 20 additions & 0 deletions app/models/challenge_assignment.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -496,4 +496,24 @@ def self.update_placeholder_assignments!(collection)
end
end
end

# create fresh new assignments for pinch hitters assigned alongside existing offer signups
def self.split_off_write_in_giver_assignments!(collection)
collection.assignments.each do |assignment|
assignment.split if assignment.double_assigned?
end
end

def double_assigned?
pinch_hitter && offer_signup && pinch_hitter.user != offering_user
end

def split
Comment on lines +507 to +511
Copy link
Member

@brianjaustin brianjaustin Jan 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🤔 )

new_assignment = self.dup
new_assignment.offer_signup = nil
new_assignment.save!

self.pinch_hitter = nil
save!
Comment on lines +514 to +517
Copy link
Member

@brianjaustin brianjaustin Jan 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

end
end
30 changes: 30 additions & 0 deletions spec/controllers/challenge_assignments_controller_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,36 @@
end
end

context "when an assignment has both an offer and a pinch hitter" do
let(:params) do
{
collection_id: collection.name,
challenge_assignments: {
assignment.id => {
request_signup_pseud: signup1.pseud.byline,
offer_signup_pseud: signup2.pseud.byline,
pinch_hitter_byline: other_user.pseuds.first.byline
}
}
}
end

it "creates a new assignment for the pinch hitter" do
PotentialMatch.create(request_signup: signup1,
offer_signup: signup2,
collection: collection)

put :set, params: params

assignment.reload
expect(assignment.pinch_hitter).to be_nil
Copy link
Member

@brianjaustin brianjaustin Jan 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add an assertion to make sure the offer signup is not cleared here? (Probably paranoid of me, but just in case 😅 )


pinch_hitter_assignment = collection.assignments.find_by(pinch_hitter_id: other_user.pseuds.first.id)
expect(pinch_hitter_assignment).to be_present
expect(pinch_hitter_assignment.offer_signup).to be_nil
end
end

context "when assignments have been sent" do
let(:gift_exchange) { create(:gift_exchange, assignments_sent_at: Faker::Time.backward) }

Expand Down
Loading