[56] Request to join groups #121
Conversation
|
|
||
| def update_counter_cache | ||
| return unless status_changed? | ||
| increment_counter_cache |
There was a problem hiding this comment.
Hmm shouldn't this actually be if accepted?? I'm just thinking about cases where it goes from requested to invited hypothetically - that shouldn't trigger an increment of the counter cache. Thoughts?
There was a problem hiding this comment.
the increment only works if the status is accepted
| # Class method to permit calling :create! on the class without instantiating | ||
| # the service object directly | ||
| # | ||
| # @param [#to_h] params The params object from the MembershipsController. |
There was a problem hiding this comment.
Let's fix this comment since there is no MembershipsController.
|
|
||
| # Initialize a MembershipCreator | ||
| # | ||
| # @param [#to_h] params The params object from the MembershipsController. |
|
|
||
| def success | ||
| { object: [object.group.draw, object.group], | ||
| msg: { success: "#{object.user.full_name} accepted." } } |
There was a problem hiding this comment.
Just a note, but we may want to make this message configurable for this object since it will likely be different for accepting invitations vs accepting requests. That will likely come into play in #107.
There was a problem hiding this comment.
We could just change it to "joined group" instead of "accepted"?
| <% if policy(@group).request_to_join? %> | ||
| <%= link_to 'Request To Join', draw_group_request_path(@draw, @group), method: :post %> | ||
| <% end %> | ||
| <%= link_to('Delete', draw_group_path(@draw, @group), method: :delete) if policy(@group).destroy? %> |
There was a problem hiding this comment.
Another note, but this feels like it will be worth extracting into a partial for the footer where we can pass in the object in question as a local variable, since destroy? is always the relevant action across all the policies. I'll open an issue for this.
| it 'succeeds' do # rubocop:disable RSpec/ExampleLength | ||
| group = FactoryGirl.create(:open_group) | ||
| user = FactoryGirl.create(:student, intent: 'on_campus', draw: group.draw) | ||
| MembershipCreator.create!(group: group, user: user, status: 'requested') |
There was a problem hiding this comment.
Should we just use Membership.create here? I'm not sure what the service object adds since we're passing all the same parameters in.
There was a problem hiding this comment.
Yea that's what we did in all the other specs, so I'd recommend changing it here.
| it 'returns an array of users who have requested to join' do | ||
| group = FactoryGirl.create(:open_group) | ||
| user = FactoryGirl.create(:student, intent: 'on_campus', draw: group.draw) | ||
| Membership.create(group: group, user: user, status: 'requested') |
There was a problem hiding this comment.
Lol, see my previous comment - we weren't even consistent last night 😛.
esoterik
force-pushed
the
56_request_to_join_groups
branch
from
45bc207
to
c966d8d
Compare
| @@ -33,6 +34,19 @@ def destroy | |||
| handle_action(**result) | |||
| end | |||
|
|
|||
| def request_to_join | |||
| result = MembershipCreator.new(group: @group, user: current_user, | |||
| status: 'requested').create! | |||
There was a problem hiding this comment.
should be changed to just call create! directly
Resolves #56 - Adds Membership creator / updater objects - Adds status to Memberships - Changes Group#memberships_count to a conditional counter cache that only counts accepted memberships towards the total
esoterik
force-pushed
the
56_request_to_join_groups
branch
from
c966d8d
to
6459441
Compare
Resolves #56