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(notifications_push_repository): Init #2482

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

provokateurin
Copy link
Member

Fixes #577
Depends on #2407 (due to an ugly cyclic dependency)

The framework part can probably be improved, but I just wanted to put out the repository so it can get reviewed.

The repository now handles all possible things that could go wrong and retries registering at UnifiedPush or Nextcloud if necessary. This should make push notifications super reliable and usable.

@provokateurin
Copy link
Member Author

Also makes it possible to implement #213 later.

@provokateurin provokateurin force-pushed the feat/notifications_push_repository branch 2 times, most recently from d3ef216 to 7d16bc0 Compare September 12, 2024 07:38
@Leptopoda
Copy link
Member

Should we land the repository and the migration changes in separate PRs?
Maybe we can land both into a temporary branch that we then merge once both are reviewed.

Both commits are already huge to begin with.

@provokateurin
Copy link
Member Author

Hm if we only have 2 PRs I'd say just merge the repository into main and then later the refactor.
A special branch is not really necessary as there are no incremental changes that need to be merged together into main (the repository is standalone after all).

@provokateurin provokateurin force-pushed the feat/notifications_push_repository branch from 7d16bc0 to e8880db Compare September 12, 2024 10:33
@provokateurin
Copy link
Member Author

I removed the framework commit and pushed it to a separate branch. Once merged I'll open a new PR for that.

@provokateurin provokateurin force-pushed the feat/notifications_push_repository branch from e8880db to fad21d1 Compare September 12, 2024 11:24
@provokateurin
Copy link
Member Author

I rebased this, but while testing I found an error with the push subscription registration.
You can already review this, but I will have to look the problem so expect some (hopefully minor) changes.

@provokateurin provokateurin force-pushed the feat/notifications_push_repository branch from fad21d1 to 7ec5abb Compare September 13, 2024 04:38
Copy link

codecov bot commented Sep 13, 2024

Codecov Report

Attention: Patch coverage is 96.78899% with 7 lines in your changes missing coverage. Please review.

Project coverage is 28.58%. Comparing base (395478f) to head (4d72c86).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
...es/neon_framework/lib/src/storage/persistence.dart 0.00% 2 Missing ⚠️
...neon_framework/lib/src/storage/settings_store.dart 0.00% 2 Missing ⚠️
...repository/lib/src/models/push_subscription.g.dart 96.72% 2 Missing ⚠️
...ository/lib/src/notifications_push_repository.dart 98.88% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2482      +/-   ##
==========================================
+ Coverage   28.47%   28.58%   +0.10%     
==========================================
  Files         352      359       +7     
  Lines      135907   136125     +218     
==========================================
+ Hits        38699    38910     +211     
- Misses      97208    97215       +7     
Flag Coverage Δ *Carryforward flag
account_repository 98.76% <ø> (ø)
cookie_store 99.48% <ø> (ø) Carriedforward from 395478f
dashboard_app 96.05% <ø> (ø)
dynamite 31.08% <ø> (ø) Carriedforward from 395478f
dynamite_end_to_end_test 61.69% <ø> (ø) Carriedforward from 395478f
dynamite_runtime 85.40% <ø> (ø) Carriedforward from 395478f
neon_dashboard 96.05% <ø> (ø) Carriedforward from 395478f
neon_framework 57.01% <0.00%> (-0.07%) ⬇️
neon_http_client 97.50% <ø> (ø) Carriedforward from 395478f
neon_notifications 100.00% <ø> (ø) Carriedforward from 395478f
neon_talk 99.45% <ø> (ø) Carriedforward from 395478f
nextcloud 24.26% <ø> (ø) Carriedforward from 395478f
notifications_app 100.00% <ø> (ø)
notifications_push_repository 98.59% <98.59%> (?)
sort_box 90.90% <ø> (ø) Carriedforward from 395478f
talk_app 99.09% <ø> (ø)

*This pull request uses carry forward flags. Click here to find out more.

Files with missing lines Coverage Δ
packages/neon_framework/lib/src/testing/mocks.dart 92.30% <ø> (ø)
...h_repository/lib/src/models/push_subscription.dart 100.00% <100.00%> (ø)
...repository/lib/src/notifications_push_storage.dart 100.00% <100.00%> (ø)
...ory/lib/src/testing/testing_push_notification.dart 100.00% <100.00%> (ø)
...ory/lib/src/testing/testing_push_subscription.dart 100.00% <100.00%> (ø)
...ions_push_repository/lib/src/utils/encryption.dart 100.00% <100.00%> (ø)
...ository/lib/src/notifications_push_repository.dart 98.88% <98.88%> (ø)
...es/neon_framework/lib/src/storage/persistence.dart 71.42% <0.00%> (-28.58%) ⬇️
...neon_framework/lib/src/storage/settings_store.dart 84.61% <0.00%> (-15.39%) ⬇️
...repository/lib/src/models/push_subscription.g.dart 96.72% <96.72%> (ø)

Copy link
Member

@Leptopoda Leptopoda left a comment

Choose a reason for hiding this comment

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

Thanks for the work.
I'm not a big fan of the code style in the repo.
Many methods in there do not take any parameters and rely on the global state of the repo. I've already outlined most of my concerns, and maybe fixing them is enough.

Comment on lines 59 to 63
final Serializers _serializers = (Serializers().toBuilder()
..add(notifications.DecryptedSubject.serializer)
..add(PushNotification.serializer)
..addPlugin(StandardJsonPlugin()))
.build();
Copy link
Member

Choose a reason for hiding this comment

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

Why don't you use a separate file with a single serializer for every model?

Copy link
Member Author

Choose a reason for hiding this comment

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

This was just copied from the framework. I can change it 👍

Copy link
Member Author

@provokateurin provokateurin Sep 23, 2024

Choose a reason for hiding this comment

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

I don't think I get what you mean 🤔

@provokateurin provokateurin force-pushed the feat/notifications_push_repository branch from 7ec5abb to 4d72c86 Compare September 23, 2024 14:11
@provokateurin provokateurin marked this pull request as ready for review September 23, 2024 14:12
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.

Refactor push notification registration
2 participants