Skip to content
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.

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

fix: [M3-8409] - Community Notifications in Event Messages v2 Refactor #10742

Conversation

pmakode-akamai
Copy link
Contributor

@pmakode-akamai pmakode-akamai commented Aug 2, 2024

Description 📝

Improve Community Like Notifications in Event Messages v2 Refactor.

When an answer/question is "liked" on the Community Questions site, the person wrote that answer/question is sent a notification in Cloud Manager. However, the syntax for that notification doesn't really make sense in Event Messages v2:
v2 Events Landing showing username and full message

Note

Default 'Linode' is kept as it is because it was already defined as a fallback when the username is null in Event Messages v2.

Changes 🔄

In Event Messages v2:

  • Exclude usernames from community like events.
  • Add username getter utility that returns usernames from event or 'Linode' if username is null or excluded by action.
  • Update the notification message for community like events to ensure it is grammatically correct.

Target release date 🗓️

8/19

Preview 📷

Before (v2) After (v2)
v2 notification menu before v2 notfication menu after
v2 events landing page before v2 events landing page after

How to test 🧪

Reproduction steps

With the Event Messages v2 flag ON locally:

  • Get a user who is not you to like one of your answers and the question on the Community Questions site. This generates the event notification.

Verification steps

  • Verify that the notification message for community like events for both question and answers are grammatically correct in Event Messages v2.
  • For both the 'Events Landing page' and 'Notification Menu':
    • Verify that Event Messages v1 will not display the username in notification message for actions added in ACTIONS_WITHOUT_USERNAMES or when username is null.
    • Verify Event Messages v2 will display 'Linode' as a fallback for actions added in ACTIONS_WITHOUT_USERNAMES or when username is null.

As an Author I have considered 🤔

Check all that apply

  • 👀 Doing a self review
  • ❔ Our contribution guidelines
  • 🤏 Splitting feature into small PRs
  • ➕ Adding a changeset
  • 🧪 Providing/Improving test coverage
  • 🔐 Removing all sensitive information from the code and PR description
  • 🚩 Using a feature flag to protect the release
  • 👣 Providing comprehensive reproduction steps
  • 📑 Providing or updating our documentation
  • 🕛 Scheduling a pair reviewing session
  • 📱 Providing mobile support
  • ♿ Providing accessibility support

@pmakode-akamai pmakode-akamai self-assigned this Aug 2, 2024
@pmakode-akamai pmakode-akamai requested a review from a team as a code owner August 2, 2024 12:18
@pmakode-akamai pmakode-akamai requested review from hana-akamai and jaalah-akamai and removed request for a team August 2, 2024 12:18
@pmakode-akamai pmakode-akamai marked this pull request as draft August 2, 2024 12:19
@pmakode-akamai pmakode-akamai changed the title fix: [M3-8409] - Community Like Notifications in event messages V2 fix: [M3-8409] - Community Like Notifications in Event Messages V2 Aug 2, 2024
@pmakode-akamai pmakode-akamai changed the title fix: [M3-8409] - Community Like Notifications in Event Messages V2 fix: [M3-8409] - Community Like Notifications in Event Messages v2 Refactor Aug 2, 2024
Copy link

github-actions bot commented Aug 2, 2024

Coverage Report:
Base Coverage: 82.41%
Current Coverage: 82.41%

@pmakode-akamai pmakode-akamai force-pushed the M3-8409-fix-v2-community-like-notifications-in-event-messages branch from f5bbe81 to 2cee22e Compare August 2, 2024 17:06
@pmakode-akamai pmakode-akamai marked this pull request as ready for review August 2, 2024 17:07
@mjac0bs mjac0bs self-requested a review August 2, 2024 17:33
Copy link
Contributor

@mjac0bs mjac0bs left a comment

Choose a reason for hiding this comment

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

Thanks for the follow up, @pmakode-akamai.

Confirmed that this is looking good for v2 community_like events. I'd forgotten that we'd added a default of Linode - works well here. We'll just want a unit test for the new util.

Screenshot 2024-08-02 at 1 10 56 PM

Screenshot 2024-08-02 at 1 11 11 PM

I'm tempted to say that we should add community_question_reply and community_mention events on the ACTIONS_WITHOUT_USERNAMES too, since those are events generated by people other than the user, and Linode is informing the user of them - not sure if @jaalah-akamai or others have thoughts on that.

@pmakode-akamai pmakode-akamai force-pushed the M3-8409-fix-v2-community-like-notifications-in-event-messages branch from 2cee22e to 910f86d Compare August 3, 2024 10:05
@pmakode-akamai
Copy link
Contributor Author

pmakode-akamai commented Aug 5, 2024

I'm tempted to say that we should add community_question_reply and community_mention events on the ACTIONS_WITHOUT_USERNAMES too, since those are events generated by people other than the user, and Linode is informing the user of them - not sure if @jaalah-akamai or others have thoughts on that.

@mjac0bs As discussed with Jaalah before, we wanted to exclude usernames from community_like events only, as likes (from payload) could come from multiple people for the same question or answer within the 3-min cron job window.

What are your thoughts on this @jaalah-akamai? As suggested by @mjac0bs, Should the username be excluded from community_mention and community_question_reply events as well?

@jaalah-akamai
Copy link
Contributor

Yea I agree with @mjac0bs

@pmakode-akamai
Copy link
Contributor Author

pmakode-akamai commented Aug 5, 2024

Yea I agree with @mjac0bs

Okay. Excluded usernames from community_mention and community_question_reply events, too.

@pmakode-akamai pmakode-akamai changed the title fix: [M3-8409] - Community Like Notifications in Event Messages v2 Refactor fix: [M3-8409] - Community Notifications in Event Messages v2 Refactor Aug 5, 2024
Copy link
Contributor

@mjac0bs mjac0bs left a comment

Choose a reason for hiding this comment

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

The update to include all community events as "without username" looks good on the v2 end.

Minor: I checked for regressions with v1 and noticed that there's a tiny formatting issue with community_question_reply - a space before the period because we seem to be formatting links that way, which is also gets rid of the quotes in the v1 community_question_reply. That message we define is: There has been a reply to your thread "${e.entity.label}".

It would more closely match the community_mention and fix the space issue if we tweaked it to be: There has been a reply to your thread: ${e.entity.label}.- the period would get replaced by the space. That might be the easiest fix, because although I'm not sure when we plan on turning Events v2 on in prod, my thought is that v1 won't be around much longer.

Screenshot 2024-08-05 at 1 52 16 PM

@mjac0bs mjac0bs added the Add'tl Approval Needed Waiting on another approval! label Aug 5, 2024
@pmakode-akamai pmakode-akamai force-pushed the M3-8409-fix-v2-community-like-notifications-in-event-messages branch from aefbb4b to 3c734db Compare August 6, 2024 07:10
@bnussman-akamai bnussman-akamai removed the Add'tl Approval Needed Waiting on another approval! label Aug 7, 2024
@bnussman-akamai bnussman-akamai added the Approved Multiple approvals and ready to merge! label Aug 7, 2024
@pmakode-akamai pmakode-akamai force-pushed the M3-8409-fix-v2-community-like-notifications-in-event-messages branch from 3c734db to 2900609 Compare August 9, 2024 08:52
@pmakode-akamai pmakode-akamai merged commit 1e2f613 into linode:develop Aug 9, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approved Multiple approvals and ready to merge!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants