Move Notification Logic from Discussion Store to Firebase Functions #3620

goratt12 · 2024-06-03T21:06:48Z

…ebase functions

PR Checklist

- Commit messages are descriptive, it will be used in our Release Notes

PR Type

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Developer experience (improves developer workflows for contributing to the project)

Description

Move Notification Logic from Discussion Store to Firebase Functions
Added tests

Git Issues

Closes #3563

Screenshots/Videos

If useful, provide screenshot or capture to highlight main changes

What happens next?

Thanks for the contribution! We try to make sure all PRs are reviewed ahead of our monthly maintainers call (first Monday of the month)

If the PR is working as intended it'll be merged and included in the next platform release, if not changes will be requested and re-reviewed once updated.

If you need more immediate feedback you can try reaching out on Discord in the Community Platform development channel.

…ebase functions

goratt12 · 2024-06-03T21:08:10Z

Tested manually and it's working.
tested on both research and questions - nested and regular comments.

github-actions · 2024-06-03T22:03:40Z

Visit the preview URL for this PR (updated for commit 2997735):

https://onearmy-next--pr3620-3563-move-discussion-0q62o5e1.web.app

_{(expires Wed, 31 Jul 2024 10:24:30 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

_{Sign: 6d65e4f8fee2f6ab2da0c1c3b85b8797d66afa59}

…tions

cypress · 2024-06-04T20:38:01Z

3 failed tests on run #5734 ↗︎

3	74	1	0	0

Details:

fix(functions): changed IDiscussionComment to IComment
Project: onearmy-community-platform	Commit: `570d80785b`
Status: Failed	Duration: 05:02 💡
Started: Jun 4, 2024 8:32 PM	Ended: Jun 4, 2024 8:38 PM

src/integration/questions/discussions.spec.ts • 1 failed test • ci-chrome

View Output Video

Test		Artifacts
[Questions.Discussions] > allows authenticated users to contribute to discussions		`Test Replay` `Screenshots` `Video`

src/integration/howto/discussions.spec.ts • 1 failed test • ci-chrome

View Output Video

Test		Artifacts
[Howto.Discussions] > allows authenticated users to contribute to discussions		`Test Replay` `Screenshots` `Video`

src/integration/research/discussions.spec.ts • 1 failed test • ci-chrome

View Output Video

Test		Artifacts
[Research.Discussions] > allows authenticated users to contribute to discussions		`Test Replay` `Screenshots` `Video`

Review all test suite changes for PR #3620 ↗︎

benfurber

Nice one. Just leaving some code level comments initially. Especially as the cypress tests are failing (which I think we should expect as the functions the ci env are running won't include these changes) I'll checkout the branch and test myself.

functions/src/Utils/db.utils.ts

benfurber · 2024-06-05T14:59:40Z

functions/src/discussionUpdates/index.test.ts

+      )
+    })
+
+    it('create nested comment and notify parent comment creator - research', async () => {


As you have multiple tests that relate to research, then questions, etc. these are the right use case for a nested describle block, literally just with the name 'Research'

Should I change the title from 'create nested comment and notify parent comment creator - research' to 'create nested comment and notify parent comment creator'? without the ' - research'

functions/src/discussionUpdates/index.test.ts

cypress · 2024-06-05T20:00:55Z

1 failed and 1 flaky tests on run #5904 ↗︎

1	66	1	0	1

Details:

Merge branch 'master' into 3563-move-discussion-notifications-to-functions
Project: onearmy-community-platform	Commit: `2997735809`
Status: Failed	Duration: 07:36 💡
Started: Jul 1, 2024 10:54 AM	Ended: Jul 1, 2024 11:02 AM

src/integration/howto/write.spec.ts • 1 failed test • ci-chrome

View Output Video

Test		Artifacts
[How To] > [Create a how-to] > [By Authenticated]		`Test Replay` `Screenshots` `Video`

src/integration/settings.spec.ts • 1 flaky test • ci-chrome

View Output Video

Test		Artifacts
[Settings] > [Focus Community Builder] > [Edit a new profile]		`Test Replay` `Screenshots` `Video`

Review all test suite changes for PR #3620 ↗︎

benfurber · 2024-06-07T09:29:42Z

I've deployed these functions to the ci env. Will rerun the tests now and hopefully all will pass...

benfurber · 2024-06-07T12:21:16Z

@goratt12. Tests still failing unfortunately. Can I leave it to you please to confirm the functions deployed as expected, etc. and make changes accordingly?

goratt12 · 2024-06-07T19:33:39Z

@goratt12. Tests still failing unfortunately. Can I leave it to you please to confirm the functions deployed as expected, etc. and make changes accordingly?

Could you please give me permissions to the onearmy-test-ci Firebase project?

…tions

codecov · 2024-06-13T15:38:14Z

Codecov Report

Attention: Patch coverage is 95.58824% with 3 lines in your changes missing coverage. Please review.

Project coverage is 74.96%. Comparing base (1db280e) to head (3a9817c).

❗ Current head 3a9817c differs from pull request most recent head fe8a972

Please upload reports for the commit fe8a972 to get more accurate results.

Files	Patch %	Lines
functions/src/discussionUpdates/index.ts	94.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3620      +/-   ##
==========================================
+ Coverage   73.68%   74.96%   +1.27%     
==========================================
  Files          41       42       +1     
  Lines        1254     1322      +68     
  Branches      253      279      +26     
==========================================
+ Hits          924      991      +67     
- Misses        308      309       +1     
  Partials       22       22

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

benfurber · 2024-06-14T08:57:43Z

@goratt12 The test comment issue, can you give more of the detail we talked about yesterday please.

Looks like you've still got a merge conflict? Happy to jump on a final manual test once that's sorted.

…tions

functions/src/discussionUpdates/index.ts

mariojsnunes · 2024-06-14T14:48:00Z

For the tests, if they are now invoking cloud functions, maybe we could mock the request?
Did that recently here:
https://github.com/mariojsnunes/community-platform/blob/4f0e648e56a14a9560362879561798c22551bb9e/packages/cypress/src/integration/profile.spec.ts#L18-L21

goratt12 · 2024-06-14T15:00:21Z

For the tests, if they are now invoking cloud functions, maybe we could mock the request? Did that recently here: https://github.com/mariojsnunes/community-platform/blob/4f0e648e56a14a9560362879561798c22551bb9e/packages/cypress/src/integration/profile.spec.ts#L18-L21

That's a good idea, but the functions are internally triggered by Firestore changes, so we cannot intercept them there. The problem is that the function handleDiscussionUpdate listens for changes at this path: discussions_rev20231022/{id}. In our E2E tests, the Firestore collection is {run_id}_discussions_rev20231022. We considered changing the function to listen for changes with a wildcard path, but we decided that altering the environment for the tests is not the correct approach.

benfurber · 2024-06-24T09:25:35Z

@goratt12 We decided what to do about this didn't we? If we can't test it without big setup changes, we can't test it without big setup changes.

goratt12 · 2024-06-25T04:50:11Z

@goratt12 We decided what to do about this didn't we? If we can't test it without big setup changes, we can't test it without big setup changes.

Agreed, so currently I'm leaving the relevant tests commented with the explanation

…tions

feat(functions): move notification logic from discussion store to fir…

ed981c7

…ebase functions

goratt12 requested a review from a team as a code owner June 3, 2024 21:06

github-actions bot added the Backend label Jun 3, 2024

davehakkens added the Review allow-preview ✅ Has received manual check for malicious code and can be safely built for preview label Jun 3, 2024

goratt12 added 4 commits June 4, 2024 19:58

fix(functions): removed old tests

37ec4b9

Merge branch 'master' into 3563-move-discussion-notifications-to-func…

e847de0

…tions

fix(functions): fixed lint issues due to merge

a6fa28d

fix(functions): changed IDiscussionComment to IComment

570d807

benfurber reviewed Jun 5, 2024

View reviewed changes

fix(functions): addressed change requests

a770ef7

feat(functions): prevent duplicate notifications

990203b

goratt12 added 2 commits June 13, 2024 15:19

fix(functions): comment out notification tests in discussion specs

d523fef

Merge branch 'master' into 3563-move-discussion-notifications-to-func…

13e4afe

…tions

Merge branch 'master' into 3563-move-discussion-notifications-to-func…

471917c

…tions

mariojsnunes reviewed Jun 14, 2024

View reviewed changes

functions/src/discussionUpdates/index.ts Outdated Show resolved Hide resolved

fix(functions): fix requested changes

2c64f01

benfurber added the 🤝 Awaiting author Waiting on action from the author label Jun 24, 2024

fix(functions): fixed lint issues

3a9817c

benfurber added 2 commits June 25, 2024 13:07

Merge branch 'master' into 3563-move-discussion-notifications-to-func…

fe8a972

…tions

Merge branch 'master' into 3563-move-discussion-notifications-to-func…

2997735

…tions

benfurber closed this Nov 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move Notification Logic from Discussion Store to Firebase Functions #3620

Move Notification Logic from Discussion Store to Firebase Functions #3620

goratt12 commented Jun 3, 2024

goratt12 commented Jun 3, 2024

github-actions bot commented Jun 3, 2024 •

edited

Loading

cypress bot commented Jun 4, 2024

benfurber left a comment

benfurber Jun 5, 2024

goratt12 Jun 5, 2024

cypress bot commented Jun 5, 2024 •

edited

Loading

benfurber commented Jun 7, 2024

benfurber commented Jun 7, 2024

goratt12 commented Jun 7, 2024

codecov bot commented Jun 13, 2024 •

edited

Loading

benfurber commented Jun 14, 2024

mariojsnunes commented Jun 14, 2024

goratt12 commented Jun 14, 2024

benfurber commented Jun 24, 2024

goratt12 commented Jun 25, 2024

Move Notification Logic from Discussion Store to Firebase Functions #3620

Move Notification Logic from Discussion Store to Firebase Functions #3620

Conversation

goratt12 commented Jun 3, 2024

Description

Git Issues

Screenshots/Videos

What happens next?

goratt12 commented Jun 3, 2024

github-actions bot commented Jun 3, 2024 • edited Loading

cypress bot commented Jun 4, 2024

3 failed tests on run #5734 ↗︎

Review all test suite changes for PR #3620 ↗︎

benfurber left a comment

Choose a reason for hiding this comment

benfurber Jun 5, 2024

Choose a reason for hiding this comment

goratt12 Jun 5, 2024

Choose a reason for hiding this comment

cypress bot commented Jun 5, 2024 • edited Loading

1 failed and 1 flaky tests on run #5904 ↗︎

Review all test suite changes for PR #3620 ↗︎

benfurber commented Jun 7, 2024

benfurber commented Jun 7, 2024

goratt12 commented Jun 7, 2024

codecov bot commented Jun 13, 2024 • edited Loading

Codecov Report

benfurber commented Jun 14, 2024

mariojsnunes commented Jun 14, 2024

goratt12 commented Jun 14, 2024

benfurber commented Jun 24, 2024

goratt12 commented Jun 25, 2024

github-actions bot commented Jun 3, 2024 •

edited

Loading

cypress bot commented Jun 5, 2024 •

edited

Loading

codecov bot commented Jun 13, 2024 •

edited

Loading