Skip to content

[PM-2901] Synchronize sends on send creation/update/deletion notification#2606

Merged
aj-rosado merged 9 commits intobitwarden:masterfrom
quexten:feature/sync-on-send-changed
Aug 8, 2023
Merged

[PM-2901] Synchronize sends on send creation/update/deletion notification#2606
aj-rosado merged 9 commits intobitwarden:masterfrom
quexten:feature/sync-on-send-changed

Conversation

@quexten
Copy link
Contributor

@quexten quexten commented Jul 9, 2023

Type of change

  • Bug fix
  • Tech debt (refactoring, code cleanup, dependency upgrades, etc)
  • New feature development
  • Build/deploy pipeline (DevOps)
  • Other

Objective

Fixes #2571. On desktop/web, sends are synced when the Websocket notification notifying the clients of the creation/update/deletion arrives. On mobile this is not implemented. This PR brings feature parity in that regard by implementing syncing send creation/updates/deletions on arrival of the Firebase notification.

Beware, I was not able to fully test this as the default config builds for the QA push environment.

Code changes

  • PushNotificationListenerService.cs Add code handling the 3 send notification types (creation, update, deletion). The code is pretty much the same as for ciphers.
  • NotificationResponse.cs Add a class to contain send sync notification data
  • ISyncService.cs, SyncService.cs Add code for doing a sync for sends.

Before you submit

  • Please check for formatting errors (dotnet format --verify-no-changes) (required)
  • Please add unit tests where it makes sense to do so (encouraged but not required)
  • If this change requires a documentation update - notify the documentation team
  • If this change has particular deployment requirements - notify the DevOps team

@bitwarden-bot
Copy link

Thank you for your contribution! We've added this to our internal Community PR board for review.
ID: PM-2901

@bitwarden-bot bitwarden-bot changed the title Synchronize sends on send creation/update/deletion notification [PM-2901] Synchronize sends on send creation/update/deletion notification Jul 9, 2023
@aj-rosado aj-rosado self-assigned this Aug 3, 2023
Copy link
Contributor

@aj-rosado aj-rosado left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution @quexten , I really appreciate your work on this

I'd like to request a minor adjustment in the notification type validation.

While the code follows Cipher's logic, I have a few suggestions to further enhance readability and reduce indentation. If you could implement these changes, I would be sincerely grateful! 😃

Copy link
Contributor

@aj-rosado aj-rosado left a comment

Choose a reason for hiding this comment

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

Thank you for your efforts cleaning the code 👏

Just requesting two more changes.

Co-authored-by: aj-rosado <109146700+aj-rosado@users.noreply.github.com>
aj-rosado
aj-rosado previously approved these changes Aug 4, 2023
Copy link
Contributor

@aj-rosado aj-rosado left a comment

Choose a reason for hiding this comment

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

Looking good! Thank you for your contribution and the efforts in improving it.

Sending to QA 🎉

Copy link
Contributor

@aj-rosado aj-rosado left a comment

Choose a reason for hiding this comment

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

Reapproving it, just committed a minor fix on whitespace.

@aj-rosado aj-rosado removed the needs-qa label Aug 8, 2023
@aj-rosado aj-rosado merged commit eea7c6b into bitwarden:master Aug 8, 2023
@aj-rosado
Copy link
Contributor

aj-rosado commented Aug 8, 2023

Thank you for your contribution @quexten! This has been merged and will be part of a future release! 🎉 🥳

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Sends not syncing automatically on mobile

3 participants