refactor: common discussion approach #3519

benfurber · 2024-05-09T14:28:25Z

Description

Until last week I hadn't really appreciated how differently the implementation for discussions on research is from the implementation for discussions on questions.

On questions, the store and component responsibilities are very seperate while for research, the responibilities are highly coupled.

This is a starting point to have a generalised approach to discussions which should make adding discussions to how-tos so much easier.

BREAKING CHANGE: No comment mentions for research. Will add a generalised approach later on. Discussed with Dave.

Now:

To-do:

Fix the show/hide button logic
Research notifications
Fix all the tests, obvs
Fix the minor design differences
Add unit test coverage for DiscussionWrapper, HideDiscussionContainer, ResearchDisscussion, QuestionDiscussion

Before releasing:

Ensure all the prod comments data is in the right places!
Add primaryContentId to prod DBs

Write-up tickets:

Simplify DiscussionContainer props
Implement for how-tos
Add custom assertations for comments, replies, notifications
Add mentions logic to discussion store
Change discussion store to service

mariojsnunes · 2024-05-09T16:08:57Z

#3165
Might want to consider the service approach

src/common/DiscussionWrapper.tsx

cypress · 2024-05-16T11:22:52Z

1 flaky test on run #5643 ↗︎

0	78	1	0	1

Details:

Merge branch 'master' into refactor/common-discussion-approach
Project: onearmy-community-platform	Commit: `41efdaea43`
Status: Passed	Duration: 04:46 💡
Started: May 21, 2024 12:10 PM	Ended: May 21, 2024 12:14 PM

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

View Output Video

Test		Artifacts
[Research] > [Create research article] > [By Authenticated]		`Test Replay` `Screenshots` `Video`

Review all test suite changes for PR #3519 ↗︎

benfurber · 2024-05-17T16:15:21Z

src/stores/Discussions/discussions.store.tsx

@@ -201,7 +209,7 @@ export class DiscussionStore extends ModuleStore {
    }
  }

-  private async _addNotification(
+  private async _addNotifications(


The plan is to move this to a cloud function anyway, so the mess in this function is temporary.

mariojsnunes

Some overall comments:

The discussionsStore doesn't really store anything related do discussions, it just calls firebase and the data is stored in the DiscussionWrapper. Other than calling firebase, the only use of the store is to access the activeUser from the RootStore.
If we call the userStore at the DiscussionWrapper to access the activeUser, we could remove the dependency on RootStore and mobx and rename the class to DiscussionService.
QuestionDiscussion and ResearchComments don't do much now, maybe can be removed in favor of using DiscussionWrapper directly?
Maybe the #update- logic could be simplified by just using the commentId and if the comment/discussion access the location hash directly.

src/common/DiscussionWrapper.tsx

src/common/HideDiscussionContainer.tsx

mariojsnunes · 2024-05-18T05:46:32Z

src/common/DiscussionWrapper.tsx

+  const { discussionStore } = useCommonStores().stores
+  const highlightedCommentId = window.location.hash.replace('#comment:', '')
+
+  const transformComments = (discussion) => {


maybe there could be a const comments = useMemo(() => ,[discussion, activeUser]) that automatically transforms the comments when the discussion or activeUser changes.
So when there is a discussion update, you simply update the discussion state, the useMemo takes care of the rest and can pass the comments to the discussionProps.

codecov · 2024-05-20T16:28:19Z

Codecov Report

Attention: Patch coverage is 71.62162% with 42 lines in your changes are missing coverage. Please review.

Project coverage is 68.01%. Comparing base (d8f9c8c) to head (41efdae).
Report is 7 commits behind head on master.

Files	Patch %	Lines
src/common/DiscussionWrapper.tsx	59.01%	24 Missing and 1 partial ⚠️
src/stores/Discussions/discussions.store.tsx	54.28%	16 Missing ⚠️
src/stores/Discussions/discussionEvents.ts	94.73%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3519      +/-   ##
==========================================
- Coverage   68.10%   68.01%   -0.10%     
==========================================
  Files         441      443       +2     
  Lines       13973    13871     -102     
  Branches     2507     2493      -14     
==========================================
- Hits         9516     9434      -82     
+ Misses       4408     4389      -19     
+ Partials       49       48       -1

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

benfurber · 2024-05-21T07:59:10Z

@mariojsnunes From your comments:

Moving QuestionStore to QuestionService. I'm with you on that approach. I'll do a ticket for it so this large branch doesn't have to get larger.
On the size of QuestionDiscussion and ResearchDiscussion. Maybe. But I'd rather keep moving in the direction of getting components smaller before deciding whether some are too small.
The #update- logic. Hadn't engaged with it at all in this PR.
useMemo Would you be up for applying that to DiscussionWrapper in a follow-up PR? I spend some time on it yesterday and just made a mess.

github-actions · 2024-05-22T09:14:35Z

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

https://onearmy-next--pr3519-refactor-common-disc-32cd70a3.web.app

_{(expires Fri, 21 Jun 2024 13:52:44 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

_{Sign: 6d65e4f8fee2f6ab2da0c1c3b85b8797d66afa59}

onearmy-bot · 2024-05-22T13:48:34Z

🎉 This PR is included in version 1.182.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

onearmy-bot · 2024-05-26T12:03:16Z

🎉 This PR is included in version 1.185.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

github-actions bot added the Mod: Research 🔬 label May 9, 2024

benfurber mentioned this pull request May 9, 2024

fix(research): use the getter from the research store to get the more correct count of all the comments for a research article #3513

Closed

5 tasks

codisart reviewed May 10, 2024

View reviewed changes

src/common/DiscussionWrapper.tsx Show resolved Hide resolved

codisart reviewed May 10, 2024

View reviewed changes

src/common/DiscussionWrapper.tsx Outdated Show resolved Hide resolved

benfurber force-pushed the refactor/common-discussion-approach branch 3 times, most recently from e35a55e to 8f66318 Compare May 16, 2024 11:06

benfurber force-pushed the refactor/common-discussion-approach branch 4 times, most recently from fbd085e to c133b6f Compare May 17, 2024 16:02

benfurber commented May 17, 2024

View reviewed changes

benfurber force-pushed the refactor/common-discussion-approach branch from c133b6f to 6257533 Compare May 17, 2024 16:23

mariojsnunes reviewed May 18, 2024

View reviewed changes

benfurber added 10 commits May 20, 2024 09:39

feat: drop comment functions from research store

3faeb36

feat: add generic discussion wrapper

87141e5

feat: use discussion wrapper for questions

d6da1b1

feat: use discussion wrapper for research

5c1e32f

test: update research comment specs

bab762e

test: fix or drop due to changed responsibilities

64dbce7

fix: setting discussion metadata for research

a79e449

feat: add research notification to discussion

30bdc05

feat: drop comments from research model

a1a0d01

test: update data mocks

e543de8

benfurber force-pushed the refactor/common-discussion-approach branch from 6257533 to 32e12f3 Compare May 20, 2024 09:30

benfurber marked this pull request as ready for review May 20, 2024 13:32

benfurber requested a review from a team as a code owner May 20, 2024 13:32

test: refactor and increase discussions coverage

111865b

benfurber force-pushed the refactor/common-discussion-approach branch from c71e5f5 to b257705 Compare May 20, 2024 16:06

benfurber added 2 commits May 20, 2024 17:11

chore: update discussion wrapper for readability

fc9580a

fix: cy variable naming in generic component

0d18648

benfurber force-pushed the refactor/common-discussion-approach branch from b257705 to 98350c2 Compare May 20, 2024 16:11

test: add some unit test coverage

58a9a94

benfurber force-pushed the refactor/common-discussion-approach branch from 98350c2 to 58a9a94 Compare May 21, 2024 09:34

benfurber added 2 commits May 21, 2024 10:43

Merge branch 'master' into refactor/common-discussion-approach

2211cc0

Merge branch 'master' into refactor/common-discussion-approach

41efdae

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

benfurber merged commit 3ad1e05 into ONEARMY:master May 22, 2024
22 checks passed

onearmy-bot added the released label May 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: common discussion approach #3519

refactor: common discussion approach #3519

benfurber commented May 9, 2024 •

edited

mariojsnunes commented May 9, 2024

cypress bot commented May 16, 2024 •

edited

benfurber May 17, 2024

mariojsnunes left a comment •

edited

mariojsnunes May 18, 2024

codecov bot commented May 20, 2024 •

edited

benfurber commented May 21, 2024

github-actions bot commented May 22, 2024 •

edited

onearmy-bot commented May 22, 2024

onearmy-bot commented May 26, 2024

refactor: common discussion approach #3519

refactor: common discussion approach #3519

Conversation

benfurber commented May 9, 2024 • edited

Description

Now:

mariojsnunes commented May 9, 2024

cypress bot commented May 16, 2024 • edited

1 flaky test on run #5643 ↗︎

Review all test suite changes for PR #3519 ↗︎

benfurber May 17, 2024

Choose a reason for hiding this comment

mariojsnunes left a comment • edited

Choose a reason for hiding this comment

mariojsnunes May 18, 2024

Choose a reason for hiding this comment

codecov bot commented May 20, 2024 • edited

Codecov Report

benfurber commented May 21, 2024

github-actions bot commented May 22, 2024 • edited

onearmy-bot commented May 22, 2024

onearmy-bot commented May 26, 2024

benfurber commented May 9, 2024 •

edited

cypress bot commented May 16, 2024 •

edited

mariojsnunes left a comment •

edited

codecov bot commented May 20, 2024 •

edited

github-actions bot commented May 22, 2024 •

edited