FEATURE: Send notification when member was accepted to group. (PR #7614)

GitHub

You’ve signed the CLA, nbianca. Thank you! This pull request is ready for review.

This needs a test :wink:

This also needs a test :wink:

This also needs a test :wink:

This also needs a test :wink:

Lets add some tests to ensure we don’t break things :wink:

Can we find out what broke due to https://github.com/discourse/discourse/pull/7503? We should improve our test case to account for whatever was broken due to the previous PR.

The problem with the previous pull request was that I refactored some things that I expected to be named the same on the client and server side, but they were not.

  • On server-side, we had :invitee_accepted, but on the client side there was INVITED_TYPE.
  • On server-side, we had :group_message_summary, but on the client side there was GROUP_SUMMARY_TYPE.

I will be adding the tests to ensure it will not happen again, but to be honest, it does very little testing.

I expected to be named the same on the client and server side, but they were not

I think we should fix that inconsistency as well.

We don’t want to just check for the count. The attributes of the notification are important as well.

Is there a reason for us to not import the constant so as to avoid duplication?

I preferred to do it this way for consistency. For me, it’s fine either way. In my opinion, it doesn’t matter so much since it’s a test fixture.

Usually we don’t want to hard code “magic” numbers in multiple places. If the number changes, we end up having to change it in multiple places and we might not even know which are the places that should change.

This is now conflicting :facepunch:

I would actually call this :membership_request_acceptedgroup_invite is a bit confusing as a name for this thing.

Is this not Membership accepted in {{group_name}} ?

overall this looks good sans a few minor changes, can you rebase?