Make notifications permanently dismissed #283

zanderle · 2024-04-05T16:37:22Z

POC for now

Closes #46

humitos

I think this is going in the direction we need. However, there are some changes in the logic we should make here.

From @agjohnson's comment in #46 (comment)

Implement cookie/local storage tracking of per-build notification dismissal. Subsequent page loads don't show the notification at all.

I understand we shouldn't dismiss this notification for all the PRs just by closing the notification, but only on this particular build. So, we should save the build.pk in the storage so we know on which build we shouldn't show.

That is, closing the notification on the first page, won't show it on any of the following page from the same build. Then, if the PR is rebuilt, it will show the notification again.

Besides, if the user opens a different PR preview the notification will be shown again.

Also, note that this addons shows a notification for PR builder and for stable/latest versions so the storage will affect those as well. We may want to split this notification addon (see #201) before moving forward with this or consider this scenario here.

src/notification.js

humitos

This approach looks good to me! 👍🏼

Is it ready to merge? I'm asking because it's in Draft and I'm not sure if you are still working on it or not, @zanderle

zanderle · 2024-05-15T08:27:55Z

This approach looks good to me! 👍🏼

Is it ready to merge? I'm asking because it's in Draft and I'm not sure if you are still working on it or not, @zanderle

No, I just need to use these new functions in the search addon as well.

Also are we ok with the behaviour implemented here (in terms of permanently dismissing the notification), or does that require further testing/discussion?

humitos · 2024-05-22T09:36:54Z

I think this approach is fine. I understand the local storage is based on the domain, right? In that case, dismissing a notification on a pull request won't affect latest/stable notifications, which is great.

However, dismissing a latest notification on https://docs.readthedocs.io/en/latest/ will also dismiss the stable notification on the same domain, for example when hitting https://docs.readthedocs.io/en/0.9.x/. So, I suppose the local storage key should be based on the version as well: with a postfix of this.config.versions.current.slug probably to make it explicit.

Am I correct?

humitos · 2024-05-27T15:14:03Z

We also serve multiple projects under the same domain, when they are subprojects. So, I think we should save the project-slug and version-slug in the local storage key as well.

agjohnson · 2024-05-28T17:06:06Z

Tracking this per subproject could be too granular, and tracking per version feels really granular. There is a good chance that if a user dismisses this once they will want it dismissed everywhere. If a user has to dismiss notifications for each version, it will feel like dismissal isn't working correctly.

For me, this keeps coming back to prompting the user for what they want -- dismiss for now, for this project, for all projects of parent project, etc.

For that, tracking the subproject is okay, as long as our plan for storage/data allow these patterns later.

humitos · 2024-05-29T10:03:04Z

Tracking this per subproject could be too granular, and tracking per version feels really granular. There is a good chance that if a user dismisses this once they will want it dismissed everywhere.

If the user see a notification "this is an old version" for a project and dismiss it, and then open an old version of a subproject, I think we should show the notification again. They are different projects and different versions, so the user should be warned about reading an outdated version of the subproject to avoid confusions.

That's the v1 of the implementation. In a next iteration we can work on the granularity and configuration/prompting, but we are not there yet.

agjohnson · 2024-05-29T16:36:07Z

That is fine for the latest/stable build warning, but the PR build warning is still fairly different.

As a user, I understand what I'm seeing is a PR build after clicking through from the PR once or twice. Reminding me every time, and requiring me to close the notification every PR, is what feels redundant. This notification is in a different class as it's not for reader users but for the maintainer, who interacts with the notification frequently.

humitos · 2024-05-30T10:07:55Z

Yes, PR notification and stable/latest should behave differently. In fact, I want to split this addon into different ones because we've reach this differences in behavior multiple times already and having only 1 addon for all of them makes everything harder and more confusing.

humitos · 2024-05-30T10:12:28Z

@zanderle can you implement the initial version of the dismiss that consider domain, project, version and language slug?

Behavior:

dismissing the notification won't show it again on the same project, version, language
switching any of them will make the notification to show again
dismissing the notification on PR builds, won't show it again on that PR preview

In the next iteration we will split the notification addon and consider the user configuration (e.g. dismiss all PR notifications always for all the domains, etc)

zanderle · 2024-05-30T12:03:05Z

@humitos I think I can. Let me dig in, and I'll let you know in case I run into any complications.

zanderle · 2024-06-17T15:24:23Z

@zanderle can you implement the initial version of the dismiss that consider domain, project, version and language slug?

Behavior:

dismissing the notification won't show it again on the same project, version, language

@humitos Ok, so project is covered by the domain name, and I can add this.config.versions.current.slug to the localstorage key to cover the version. How should I determine the language? projects.current.language.code or something else?

Is it ok to rely on the domain name to consider project, or should that be a separate thing as well? projects.current.slug perhaps?

agjohnson · 2024-06-17T17:22:34Z

If we want to give users notifications for each subproject, we will need to use the project slug.

I'd like to see this merged soon so we can fix this UX bug with notifications. I feel we're maybe getting deep into optimizing this up front for use cases. We instead could start with something simple, what's here now, and make dismissals more granular as we continue working on this.

This PR has felt like it was really close, what is left to be able to merge a fix for dismissing PR build notifications? Reworking methods the search addon uses, maybe tests?

zanderle · 2024-06-17T22:05:47Z

If we want to give users notifications for each subproject, we will need to use the project slug.

I'd like to see this merged soon so we can fix this UX bug with notifications. I feel we're maybe getting deep into optimizing this up front for use cases. We instead could start with something simple, what's here now, and make dismissals more granular as we continue working on this.

This PR has felt like it was really close, what is left to be able to merge a fix for dismissing PR build notifications? Reworking methods the search addon uses, maybe tests?

@agjohnson yeah, at this point what's left is changing the local storage key and some tests (although I'll have to check if cookies complicate things at all). I was going to also rework the search addon to use the same util functions, but that could also be a separate PR, if you wanted to expedite this one?

humitos · 2024-06-18T10:28:05Z

@zanderle yes, we should use those three variables for the local storage key:

Project slug: this.projects.current.slug
Project language code: this.projects.current.language.code
Version slug: this.config.versions.current.slug

to make it easier for future changes

… version slug

zanderle · 2024-06-18T14:43:10Z

@humitos this should be ready for review, except for tests. I'm trying to figure out what would be the best way to test the behaviour

humitos

It looks good to me 👍🏼 . I left some comments with suggestions. Let me know.

src/notification.js

humitos · 2024-06-20T09:16:57Z

tests/notification.test.html

+            };
+            const localStorageString = JSON.stringify(addonInformation);
+            getLocalStorageStub
+              .withArgs("readthedocs-Notification-storage-key")


Does it make sense to use addon.getLocalStorageKeyFromConfig(config) here (and other places) instead of hardcoding it?

I think it's better to hardcode it, because this way we also test that we don't get some accidental readthedocs-undefined-storage-key or something like that.

tests/notification.test.html

humitos

I did some small tests locally and it works fine to me 💯 . Thanks @zanderle for working on this feature with all the back and forth we had 😄 . I really appreciate it 🙏🏼

zanderle · 2024-06-20T19:25:25Z

Of course, my pleasure. I hope we got it right in the end 😅👍🏻

…

On 20 Jun 2024 at 17:56 +0200, Manuel Kaufmann ***@***.***>, wrote: @humitos approved this pull request. I did some small tests locally and it works fine to me 💯 . Thanks @zanderle for working on this feature with all the back and forth we had 😄 . I really appreciate it 🙏🏼 — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: ***@***.***>

…#353) Fixes #296 A few considerations here: - I used the new localStorage code that we added in #283 . This means the key for localStorage will change once this is deployed, meaning that previous recent searches won't show up anymore (and there will be an unused object in localStorage lying around). Are we ok with that, or do we want to make it backward compatible? - I didn't add any logic for removing recent searches from previous versions. If we don't add anything, this object will grow and grow and it will store recent searches of versions that might become very old. Do we want to add some kind of logic in this PR or should we do it in a separate PR? - Besides project and version, I also added language to the key (the same way we do it for notifications). Is that ok? --------- Co-authored-by: Manuel Kaufmann <[email protected]>

zanderle requested review from humitos and a team as code owners April 5, 2024 16:37

zanderle requested a review from agjohnson April 5, 2024 16:37

zanderle mentioned this pull request Apr 5, 2024

Notification: use a cookie to keep it closed for the session #46

Closed

humitos requested changes Apr 8, 2024

View reviewed changes

src/notification.js Outdated Show resolved Hide resolved

src/notification.js Outdated Show resolved Hide resolved

humitos marked this pull request as draft April 16, 2024 08:34

zanderle requested a review from humitos April 25, 2024 17:14

humitos approved these changes May 14, 2024

View reviewed changes

zanderle force-pushed the zanderle/close-notifications branch from 3a649cf to acdfec8 Compare June 17, 2024 15:11

zanderle force-pushed the zanderle/close-notifications branch from 4fe12db to 458b35b Compare June 18, 2024 14:29

zanderle added 7 commits June 18, 2024 16:33

Extract the notification close button into a function

774ac48

to make it easier for future changes

Make notifications permanently dismissed using localStorage

83415c4

Make dismissed a timestamp rather than a boolean

a2932a6

Implement methods to deal with local storage on AddonBase

5c23cce

Refactor notifications to use AddonBase local storage methods

8eb40a9

Add more comments

3e642dd

Make notification dismissal depend on project slug, language code and…

a75b0d8

… version slug

zanderle force-pushed the zanderle/close-notifications branch from 458b35b to fae02e2 Compare June 18, 2024 14:33

npm run format and build

cb9a62e

zanderle force-pushed the zanderle/close-notifications branch from fae02e2 to cb9a62e Compare June 18, 2024 14:39

humitos reviewed Jun 19, 2024

View reviewed changes

src/notification.js Show resolved Hide resolved

src/notification.js Outdated Show resolved Hide resolved

src/notification.js Outdated Show resolved Hide resolved

Add tests to check notification dismissal behaviour

52ea194

zanderle force-pushed the zanderle/close-notifications branch from edb1a9a to 52ea194 Compare June 19, 2024 19:04

Change when dismissedTimestamp is loaded and rename the method

a4b73dc

zanderle marked this pull request as ready for review June 19, 2024 20:40

humitos reviewed Jun 20, 2024

View reviewed changes

tests/notification.test.html Outdated Show resolved Hide resolved

zanderle added 3 commits June 20, 2024 14:53

Make localStorageKey lowercase

ee53df0

Remove the ability for notifications to have custom localStorageKeys

2368b19

npm run build

ff85222

zanderle requested a review from humitos June 20, 2024 12:54

humitos approved these changes Jun 20, 2024

View reviewed changes

humitos merged commit 36cf19c into main Jun 20, 2024
4 checks passed

humitos deleted the zanderle/close-notifications branch June 20, 2024 15:56

agilgur5 mentioned this pull request Jun 26, 2024

Notification: allow to disable latest and/or non-stable warning #133

Open

zanderle mentioned this pull request Jul 28, 2024

Fix recent searches when looking at different versions on same domain #353

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make notifications permanently dismissed #283

Make notifications permanently dismissed #283

zanderle commented Apr 5, 2024

humitos left a comment

humitos left a comment

zanderle commented May 15, 2024

humitos commented May 22, 2024

humitos commented May 27, 2024

agjohnson commented May 28, 2024 •

edited

Loading

humitos commented May 29, 2024 •

edited

Loading

agjohnson commented May 29, 2024

humitos commented May 30, 2024

humitos commented May 30, 2024

zanderle commented May 30, 2024

zanderle commented Jun 17, 2024 •

edited

Loading

agjohnson commented Jun 17, 2024 •

edited

Loading

zanderle commented Jun 17, 2024

humitos commented Jun 18, 2024

zanderle commented Jun 18, 2024

humitos left a comment

humitos Jun 20, 2024 •

edited

Loading

zanderle Jun 20, 2024

humitos left a comment

zanderle commented Jun 20, 2024 via email

Make notifications permanently dismissed #283

Make notifications permanently dismissed #283

Conversation

zanderle commented Apr 5, 2024

humitos left a comment

Choose a reason for hiding this comment

humitos left a comment

Choose a reason for hiding this comment

zanderle commented May 15, 2024

humitos commented May 22, 2024

humitos commented May 27, 2024

agjohnson commented May 28, 2024 • edited Loading

humitos commented May 29, 2024 • edited Loading

agjohnson commented May 29, 2024

humitos commented May 30, 2024

humitos commented May 30, 2024

zanderle commented May 30, 2024

zanderle commented Jun 17, 2024 • edited Loading

agjohnson commented Jun 17, 2024 • edited Loading

zanderle commented Jun 17, 2024

humitos commented Jun 18, 2024

zanderle commented Jun 18, 2024

humitos left a comment

Choose a reason for hiding this comment

humitos Jun 20, 2024 • edited Loading

Choose a reason for hiding this comment

zanderle Jun 20, 2024

Choose a reason for hiding this comment

humitos left a comment

Choose a reason for hiding this comment

zanderle commented Jun 20, 2024 via email

agjohnson commented May 28, 2024 •

edited

Loading

humitos commented May 29, 2024 •

edited

Loading

zanderle commented Jun 17, 2024 •

edited

Loading

agjohnson commented Jun 17, 2024 •

edited

Loading

humitos Jun 20, 2024 •

edited

Loading