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 watchlist problems #521

Merged
merged 6 commits into from
Jul 10, 2023
Merged

Fix watchlist problems #521

merged 6 commits into from
Jul 10, 2023

Conversation

danielgoldelman
Copy link
Collaborator

No description provided.

@danielgoldelman danielgoldelman linked an issue Jul 4, 2023 that may be closed by this pull request
@danielgoldelman
Copy link
Collaborator Author

pr held until new changes to 512 have been implemented

Copy link
Member

@SebastianZimmeck SebastianZimmeck left a comment

Choose a reason for hiding this comment

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

Thanks, @danielgoldelman, it works. But also please change the readme accordingly.

I'd say delete your earlier readme change and instead say:

  • When storing a Watchlist General Keyword, a user would be alerted the first time a keyword is entered or shared by a site
  • For all other keyword categories, a user would be alerted every time the keyword is collected or shared

(assuming that is how it is and that this is the terminology from the UI; in any case please describe in a few sentences how the Watchlist works using the terms from the UI)

@SebastianZimmeck SebastianZimmeck removed the request for review from jjeancharles July 6, 2023 19:45
@SebastianZimmeck
Copy link
Member

We first need to resolve the open points before we can merge here.

@SebastianZimmeck SebastianZimmeck changed the title notif first party keyword mentioned in readme #512 Fix watchlist problems Jul 7, 2023
Copy link
Collaborator

@jjeancharles jjeancharles left a comment

Choose a reason for hiding this comment

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

Reviewed the changes and everything that was fixed is working as intended!

@SebastianZimmeck
Copy link
Member

SebastianZimmeck commented Jul 10, 2023

OK, I am currently on the train, which is not ideal for testing this functionality. I will try in 11 hours or so with a more stable Internet connection.

@SebastianZimmeck SebastianZimmeck self-requested a review July 10, 2023 19:31
Copy link
Member

@SebastianZimmeck SebastianZimmeck left a comment

Choose a reason for hiding this comment

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

OK, I checked the General Keywords. The other ones I will check in more detail. We can start with the usability study.

@danielgoldelman danielgoldelman merged commit 3978051 into main Jul 10, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement new notification methodology
3 participants