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

feat(webapp): 🍰 allows mark all notifications as read #3922

Merged
merged 26 commits into from
Mar 9, 2023

Conversation

ftonato
Copy link
Contributor

@ftonato ftonato commented Oct 25, 2020

🍰 Pullrequest

This pull-request will allows all the users mark all notifications as read with one click

Preview:

mark-all-as-read-feature

Issues

Todo

  • backend:
    • mutation: have a singe mutation with both functionallities in markAsRead?
      • instead of extra mutation markAllAsRead
      • especially because for paginated notification list we may just want to mark a given array of IDs as read
  • webapp:
    • tests: only the frontend unit test is missing
  • clean up: search for Wolle

@Tirokk
Copy link
Member

Tirokk commented Oct 26, 2020

Hey @ftonato ,
your are invited now.
Sorry. Don’t know why I got over that. πŸ˜…

Please push after your acceptation again …

@ftonato
Copy link
Contributor Author

ftonato commented Oct 27, 2020

Hello,
Could some one take a look and try to figure out why my unit tests are not passing?

@ftonato ftonato requested review from Mogge and Tirokk and removed request for Mogge October 27, 2020 10:58
@Tirokk
Copy link
Member

Tirokk commented Oct 27, 2020

Sorry @ftonato !
The tests are not fully implemented yet, because I had so many other things to do.
I implement the tests these days. They fail, because of wrong settings and secrets … hope I get it done tomorrow.

BUT:

One thing I see is …

$ ./scripts/translations/sort.sh
de.json is not sorted by keys
en.json is not sorted by keys
es.json is not sorted by keys
fr.json is not sorted by keys
it.json is not sorted by keys
nl.json is not sorted by keys
pt.json is not sorted by keys
ru.json is not sorted by keys
The command "./scripts/translations/sort.sh" exited with 1.
$ ./scripts/translations/missing-keys.sh

The locales are tested with the command:

# in webapp folder
$ yarn locales

Or fixed by command:

# in webapp folder
$ yarn locales --fix

The entries in the locales have to be in alphabetical order and german language (second base) has to have the same entries as english (base).

Linting

In think you already know …

The linting is tested:

# in webapp and backend folder
$ yarn lint

Or fixed by:

# in webapp and backend folder
$ yarn lint --fix

Tomorrow

I can give you a review tomorrow.

@ftonato
Copy link
Contributor Author

ftonato commented Oct 27, 2020

I already passed by all these steps, but seems still not working =(

@Tirokk
Copy link
Member

Tirokk commented Oct 29, 2020

Hey @ftonato ,
sorry for not caring. I have to help somebody as an emergency who has problems with corona in his family. Because of this and other things unfortunately I need til Monday to find time for your PR. πŸ˜“

@Tirokk Tirokk changed the title [WIP] Allows mark all notifications as read [WIP] 🍰 Allows Mark All Notifications As Read Nov 2, 2020
@Tirokk Tirokk changed the title [WIP] 🍰 Allows Mark All Notifications As Read feat: [WIP] 🍰 Allows Mark All Notifications As Read Nov 2, 2020
- And added some empty lines for nice formating.
Tirokk
Tirokk previously requested changes Nov 2, 2020
Copy link
Member

@Tirokk Tirokk left a comment

Choose a reason for hiding this comment

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

Hey @ftonato ,

really cool and sophisticated first through !!! πŸš€πŸ’«

I did some comments and suggestions and there are some little questions open.
Please let me know if I can be of some help for clarifications.

PS: I have merge the master, have corrected one test, which I have tried to corrected successfully, and added some empty lines …
Pushed onto your PR. Hope that is okay?

backend/src/schema/resolvers/notifications.js Outdated Show resolved Hide resolved
backend/src/schema/resolvers/notifications.spec.js Outdated Show resolved Hide resolved
webapp/components/NotificationMenu/NotificationMenu.vue Outdated Show resolved Hide resolved
webapp/components/NotificationMenu/NotificationMenu.vue Outdated Show resolved Hide resolved
webapp/locales/nl.json Outdated Show resolved Hide resolved
webapp/pages/notifications/index.vue Outdated Show resolved Hide resolved
webapp/pages/notifications/index.vue Show resolved Hide resolved
webapp/pages/notifications/index.spec.js Outdated Show resolved Hide resolved
webapp/pages/notifications/index.spec.js Outdated Show resolved Hide resolved
@Tirokk
Copy link
Member

Tirokk commented Nov 2, 2020

Sorry, tests are still not working …

@Tirokk
Copy link
Member

Tirokk commented Nov 4, 2020

Now the Travis tests should work … @ftonato

@ftonato
Copy link
Contributor Author

ftonato commented Nov 5, 2020

Hey @Tirokk,

I'm not sure about the reason, but since I did git pull to get your commit (merge with master) I can't run the app.

See the image below please:

image

@Tirokk
Copy link
Member

Tirokk commented Nov 9, 2020

Hey @ftonato ,
I pulled your branch with reset --hard so that’s really your code.
I had not the error you mentioned above.

Can you try yarnor yarn install in your webapp? And then yarn dev again?

There are some errors in webapp tests in file pages/notifications/index.spec.js …
Did they run on your machine?

What do you think about my suggestions in relation to the style and design?

PS: I fixed the linting …

@Tirokk Tirokk dismissed their stale review November 16, 2022 13:18

I overtake myself

@ftonato
Copy link
Contributor Author

ftonato commented Feb 23, 2023

I'm closing outdated issues and pull-requests that are no longer relevant given how much time has passed since they were opened.

@ftonato ftonato closed this Feb 23, 2023
@Tirokk
Copy link
Member

Tirokk commented Feb 23, 2023

Sorry @ftonato this is still relevant.

The project gets much more financing and activity now.
In case you are interessted you are welcome. πŸ€—

Shall I unassign you in case you getting notifications you don't want?

@Tirokk Tirokk reopened this Feb 23, 2023
@ftonato
Copy link
Contributor Author

ftonato commented Feb 23, 2023

Don't worry, we can keep it open then.

I was assuming, since it's an old pull-request, that it wasn't updated and therefore it would be useless to keep it open (my fault, sorry)...

Note: I have a script that parses all my old issues/pull requests and closes it for me with the default message ^^

@Tirokk
Copy link
Member

Tirokk commented Feb 24, 2023

Okidoki πŸ‘πŸΌ
@ftonato

@Tirokk Tirokk assigned Mogge and unassigned Tirokk Feb 27, 2023
@Mogge Mogge changed the title feat(webapp): [WIP] 🍰 allows mark all notifications as read feat(webapp): 🍰 allows mark all notifications as read Mar 8, 2023
@Mogge Mogge requested review from Mogge and removed request for Mogge March 8, 2023 17:16
Copy link
Member

@Tirokk Tirokk left a comment

Choose a reason for hiding this comment

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

Hey @Mogge ,

cool that you brought that issue to an end. @Mogge

Thanks a lot for your effort and contribution to ocelot.social so long ago @ftonato !!! πŸ™πŸΌ
The project now takes big steps forward.

I approve πŸš€πŸš€πŸ’«πŸ’«

@Mogge Mogge merged commit 0634a08 into master Mar 9, 2023
@Mogge Mogge deleted the feature/mark-all-notification-as-read branch March 9, 2023 12:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

πŸš€ [Feature] Notifications – Mark All As Read
5 participants