[WIP] Turn off notifications for posts' reactions

[nicolas-amabile]

What type of PR is this? (check all applicable)

• Refactor
• Feature
• Bug Fix
• Documentation Update

Description

This PR adds the feature of disabling notifications for post reactions.

Related Tickets & Documents

Feature request here: #3244

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

Added to documentation?

• docs.dev.to
• readme
• no documentation needed

[CLAassistant]

All committers have signed the CLA.

[rhymes]

Hi @nicolas-amabile, thanks for the PR! I'm going to leave my feedback there are few things missing / to discuss, I always encourage people to start a discussion on new features on the issue, in this case #3244, than to start coding right away. This way both the team and the contributor knows how and what.

In this case I think your PR is incomplete:

• the receiver can be an organization, yet you're adding the field only to the user table
• receiver_data is a Hash with two fields: the class (User or Organization) and the ID, which means that receiver_data.fetch(:post_reaction_notifications, true) it's always going to return true.
• the migration that adds the field post_reaction_notifications is missing
• the UI for the organization needs a little bit of thinking
• there are no tests

If it's otherwise incomplete because you're working on it, please mark it as [WIP] so that it will be reviewed only at the end.

My suggestion is to start a conversation on the issue itself at least about the UI and then go back to the code from there.

[rhymes]

See my comment here #3391 (comment)

[nicolas-amabile]

@rhymes thank you for the feedback!
I misinterpreted the status: approved tag, my bad ✋I just added the [WIP] and I'll comment on the issue.

the receiver can be an organization, yet you're adding the field only to the user table

Yes, it makes sense to have the same feature for both, users and organizations.

receiver_data is a Hash with two fields: the class (User or Organization) and the ID, which means that receiver_data.fetch(:post_reaction_notifications, true) it's always going to return true.

Got it, typo when I extracted the logic into the shoud_send_reaction_notification function.
It should be receiver instead of receiver_data.

• the migration that adds the field post_reaction_notifications is missing
• the UI for the organization needs a little bit of thinking
• there are no tests

Agree, I'll need some help here, I've never used ruby before.

[mstruve]

Hey @nicolas-amabile!

Thank you so much for jumping in and trying to fix this issue! With that said, we are in the process of going through and cleaning up some of our PR history to help us better prioritize what we need to be focusing on. Since this PR has not been active for a while and has some conflicts I am going to close it for now. If you would still like to get this merged please feel free to resolve the conflicts, add the additional suggested content, and then open the PR back up. We would be more than happy to help push this through if you would like!

Thanks so much for contributing! Ping us anytime if you have questions!