feat: add notifications when user mentioned in how-to

[thisislawatts]

PR Checklist

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

Not yet, I will squash and tidy the commit messages after review process has been undertaken

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

Adds notification items to a users Notification drop down if they are mentioned in a How-to article description, how-to step or comment.

As part of this work:

• Refactored Howto.store to invoke a single internal method that updates the datastore.
• Moved knowledge of notification content out of the UI components and out to the application context, aim was to simplify logic within the UI component.
• Introduced more tests around howto.store public interface

Git Issues

Closes #1543

Screenshots/Videos

What a users sees when they have been mentioned within a Howto Article, this could be in the description, individual step or comments.

---

What happens next?

Thanks for the contribution! We try to make sure all PRs are reviewed ahead of a monthly dev call (first Monday of the month, open to all!).

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.

[cypress]

---

Test summary

52 • 0 • 0 • 0 • 0

---

Run details

Project	onearmy-community-platform
Status	Passed
Commit	96ef1e5
Started	Dec 1, 2022 8:25 PM
Ended	Dec 1, 2022 8:29 PM
Duration	03:08 💡
OS	Linux Ubuntu - 20.04
Browser	Chrome 108

View run in Cypress Dashboard ➡️

---

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

[thisislawatts]

thought: I wonder if we can remove these static mocks entirely in favour of using the factories 🤔 Not an item for this round of changes, but a future clean up.

[chrismclarke]

As far as I'm aware these mocks aren't really used at all (legacy code). There are also additional mocks in the oa-shared folder which are used in cypress e2e tests - I'm not sure what's best practice in terms of unit vs e2e test data, but definitely would have no issue if you wanted to tidy either of these up.

The only minor issue I can think of is handling cases where we want to ensure unit tests cover certain data conditions, e.g. in the userNotification store there is both a read and notified property, but the code doesn't allow for notifications that are both read:true and notified:false. I still went for mock data factory because I like the pattern you had set up on the research store, although I added overrides for those fields to make sure the test cases I wanted were populated.

[thisislawatts]

🥳 New tests which can be used to document the behaviour of the store.

[chrismclarke]

looks great! Really wish I had seen these before going ahead and writing tests for the refactored userNotifications store! (oh well, all good learning experiences and can merge the two together)

[github-actions]

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

https://onearmy-next--pr2014-feat-add-notificatio-q05g68p2.web.app

_{(expires Thu, 19 Jan 2023 23:58:21 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

_{Sign: 6d65e4f8fee2f6ab2da0c1c3b85b8797d66afa59}

[davehakkens]

let me know when you need a tester :)

[thisislawatts]

@davehakkens, I think it's ready for a first pass.

In the interests of releasing small changes frequently, what do you think about making this available for how-to articles first and then adding it for Research articles in a future iteration?

The reasoning here being that Research is only available to beta-testers and actively being used on the Project Kamp instance.

[davehakkens]

Seems to work here! (always so clunky to approve a howto for testing)

Up for releasing gradual. Not sure if I fully get the logic behind it, but I guess it would make the workload more distributed?

That said the main important area where @mentions would be powerful is comments. This would be the only way to have follow ups in a conversation. Very much missing right now

[thisislawatts]

@davehakkens The changes in this PR should generate a notification when you mention a user in a howto comment. The behaviour still missing here is scrolling a comment into view.

[davehakkens]

oe nicee!
A̶n̶d̶ ̶'̶s̶c̶r̶o̶l̶l̶i̶n̶g̶ ̶c̶o̶m̶m̶e̶n̶t̶ ̶i̶n̶t̶o̶ ̶v̶i̶e̶w̶'̶ ̶i̶s̶ ̶a̶ ̶b̶i̶g̶g̶e̶r̶ ̶o̶p̶e̶r̶a̶t̶i̶o̶n̶ ̶s̶i̶n̶c̶e̶ ̶c̶o̶m̶m̶e̶n̶t̶s̶ ̶d̶o̶n̶t̶ ̶h̶a̶v̶e̶ ̶i̶d̶'̶s̶ ̶y̶e̶t̶?̶
hehe nevermind i see you just added exactly that

[davehakkens]

ah and not sure if you are aware but we already have a similair behaviour in notifications for research.
When comment there the notifications sends you to the step were the comment is made with an anchor in the url like #update_3

[thisislawatts]

@davehakkens good catch on that behaviour in Research, I think it would be worth making this behaviour consistent between the implementations. That work should likely be done as part of a separate PR.

We should aim for the behaviour when clicking from a notification to a comment to be the same, regardless of whether it is a new comment or mention.

I think there's an additional visual hint we can introduce here which needs some design detailing. For reference please look at the following Stack Overflow link which will scroll you to a specific item on the page which initially has a yellow background that fades to white.
https://stackoverflow.com/questions/5767325/how-can-i-remove-a-specific-item-from-an-array/5767335#5767335

We don't have anything like that in the design language at the moment so I added a black outline, which looks a little… 🤢
The following screengrab shows a list of comments where the comment from Melvina76 has been linked to.

[davehakkens]

Thats a nice subtle one on Stackoverflow.
Not sure if you want to avoid CSS there. But this would make it visually a bit smoother for now. (until we unify the behaviour)

border: 2px dashed #000;
border-radius: 5px;
background-color: white;
margin-bottom: 20px;

[davehakkens]

You have an all white background behind comments? -looks like on your screenshot.
Because its also a bit weird that the outline doest go around the white box of comments on the bottom (hence the other css) Not a big deal though ..

[thisislawatts]

Ah good catch, the Storybook environment where I took the screengrab from defaults to a white background which is a little unhelpful when we have components being deployed into contexts with a different background.

[davehakkens]

aha that explains! Anyway looks good now here

[thisislawatts]

Ready for a functional/design re-review @davehakkens and a code review from @chrismclarke

[davehakkens]

works awesome. Very smooth 👌

[davehakkens]

Is it a big job to also add this for mentions in research/comments?
As in: Create a new issue for another contributer, or are you up for finishing all the way? 💐

[thisislawatts]

@davehakkens I am up for implementing this on Research items as well but let's introduce those changes as a separate PR rather than add it to this one. I believe that there are already enough changes in here and rolling @mention notifications out to How-to first seems like the thinnest possible slice which stills introduces a usable feature.

[davehakkens]

Nice. Was just planning on how-to (hihi) move forward with this feature. But what you suggest sounds great!

[thisislawatts]

I will start work on the research iteration once these changes have been merged.

[chrismclarke]

Thanks @thisislawatts
The changes look great to me. I had a slight issue with conflicts from the refactor in @1917 which were merged ahead of this, and so have tried rebasing onto #2014.

I'll put a block on this PR for now and shift follow-ups to the rebased pr

[davehakkens]

I will start work on the research iteration once these changes have been merged.

hey @thisislawatts, just checking in if the @mentions for research is still somewhere on your todo? (No rush, just to keep track of open developments and what needs help)

[thisislawatts]

@davehakkens Thanks for pinging on that. Do you know what the status is of the email notifications work? I figure there could be some churn in the notifications area as part of that. Whose leading that effort atm?

[chrismclarke]

@thisislawatts We're slowly moving ahead with it, we're starting more with the digest approach (summary of recent notifications either daily/weekly/monthly) instead of trying to email any time triggered.

I drafted a PR to handle the process of aggregating notifications in #2027, we have frontend opt-in via #2039, and have also had some work done on email templates on the side (as well as setting up a transactional email account).

So that leaves one PR to integrate scheduled functions to check the aggregations and prep/format the emails, and then another to handle the actual emailing (either via firebase extension or adapting the source code into our own function depending on whether we want to enable on individual platforms manually or not). Likely also will need a few minor tweaks as I'm not sure the current way the read/notified system is implemented quite matches what you guys had drafted in the RFC (TBC), some general testing, and also likely integrating the pending migrations work if we want to retroactively opt-in all users.

I've taken on those PRs up to now and reckon it will be a couple of days of work more, had planned on it last weekend but got sidetracked with build issues, so now likely at some point next month.

[onearmy-bot]

🎉 This issue has been resolved in version 1.33.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

[thisislawatts]

Thanks @chrismclarke, it was this particular point I was uncertain of:

Likely also will need a few minor tweaks as I'm not sure the current way the read/notified system is implemented quite matches what you guys had drafted in the RFC

If you are not looking to make any immediate changes to the Notification format then perhaps I can get this @mention notifications added to the Research component before you come round to working on it. I have a few hours available this week so will pick this up. 🎄 🎅🏻

[chrismclarke]

Yeah I won't be deep-diving anything for the next week or so, so anything you want to add/edit/refactor happy to review at the same time.

I remember when doing some manual tests not being sure myself exactly which of the read/notified properties made things appear in the notification list and/or make the notification icon active, possibly a candidate to add a few more tests to for clarity

I think there's a lot for generating and marking as read, maybe just one or two more to cover any notified state changes that happens from opening the list in app. But that's just what's in the back of my head, might just be that I missed something and just need to take a better look.

[davehakkens]

How are you with this one @thisislawatts?

[thisislawatts]

I will be offline this week @davehakkens but otherwise will pick it up over the weekend. Almost there with this work 📈