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

Sync bookmarks - notify user when sync paused #3913

Merged
merged 4 commits into from
Dec 1, 2023

Conversation

cmonfortep
Copy link
Contributor

@cmonfortep cmonfortep commented Nov 23, 2023

Task/Issue URL: https://app.asana.com/0/488551667048375/1206017382069028/f

Description

Notify user when sync is paused due collection limit or payload too large.

Steps to test this PR

Feature 1

  • Fresh install
  • Go to sync settings and create an account
  • Ensure you don't see any warning or notification about sync paused
  • Add a bookmark
  • Patch should work, no rate limit
  • Go to the code base, RealSyncEngine:L186 replace Success logic with:
            is Success -> {
                persisterPlugins.getPlugins().forEach {
                    it.onError(SyncErrorResponse(changes.type, COLLECTION_LIMIT_REACHED))
                }
                //persistChanges(result.data, conflictResolution)
            }
  • install app with changes
  • run the app
  • Add a bookmark to trigger a Patch
  • You should receive a system notification
  • Go to sync settings, ensure warning is present
  • Clicking on the warning should take you to Bookmarks Screen
  • Undo the change you applied in RealSyncEngine:L186
  • install app again
  • on open, will trigger a patch again
  • Notification and sync setting warning should be gone
  • Ensure no warning is present

UI changes

Before After
!(Upload before screenshot) (Upload after screenshot)

@cmonfortep
Copy link
Contributor Author

cmonfortep commented Nov 23, 2023

@cmonfortep cmonfortep force-pushed the feature/cristian/sync/feature_limit_ui branch 3 times, most recently from 0551ba6 to 3250c81 Compare November 27, 2023 21:30
@cmonfortep cmonfortep mentioned this pull request Nov 27, 2023
17 tasks
@cmonfortep cmonfortep force-pushed the feature/cristian/sync/feature_limit_ui branch 2 times, most recently from 5561575 to 987b615 Compare November 27, 2023 23:19
@cmonfortep cmonfortep marked this pull request as ready for review November 28, 2023 12:04
@cmonfortep cmonfortep force-pushed the feature/cristian/sync/feature_limit_ui branch from 987b615 to 978b0ee Compare November 29, 2023 09:00
@malmstein malmstein self-assigned this Nov 29, 2023
Copy link
Contributor

@malmstein malmstein left a comment

Choose a reason for hiding this comment

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

nice work @cmonfortep, works as expected. left a few comments

@cmonfortep cmonfortep force-pushed the feature/cristian/sync/feature_limit_ui branch from 978b0ee to 785950e Compare November 30, 2023 11:30
<!--
Note: This checklist is a reminder of our shared engineering
expectations.
The items in Bold are required
If your PR involves UI changes:
1. Upload screenshots or screencasts that illustrate the changes before
/ after
2. Add them under the UI changes section (feel free to add more columns
if needed)
If your PR does not involve UI changes, you can remove the **UI
changes** section

At a minimum, make sure your changes are tested in API 23 and one of the
more recent API levels available.
-->

Task/Issue URL:
https://app.asana.com/0/488551667048375/1206017382069031/f

### Description
Notify user when sync is paused due collection limit or payload too
large.

### Steps to test this PR

_Feature 1_
- [x] Fresh install
- [x] Go to sync settings and create an account
- [x] Ensure you don't see any warning or notification about sync paused
- [x] Add a Login
- [x] Patch should work, no rate limit
- [x] Go to the code base, `RealSyncEngine:L186` replace Success logic
with:
```
            is Success -> {
                persisterPlugins.getPlugins().forEach {
                    it.onError(SyncErrorResponse(changes.type, COLLECTION_LIMIT_REACHED))
                }
                //persistChanges(result.data, conflictResolution)
            }
```
- [x] install app with changes
- [x] run the app
- [x] Add a Login to trigger a Patch
- [x] You should receive a system notification
- [x] Go to sync settings, ensure warning is present
- [x] Clicking on the warning should take you to Logins Screen
- [x] Undo the change you applied in `RealSyncEngine:L186`
- [x] install app again
- [x] on open, will trigger a patch again
- [x] Notification and sync setting warning should be gone
- [x] Ensure no warning is present

### UI changes
| Before  | After |
| ------ | ----- |
!(Upload before screenshot)|(Upload after screenshot)|
@cmonfortep cmonfortep merged commit eee77e1 into develop Dec 1, 2023
5 checks passed
@cmonfortep cmonfortep deleted the feature/cristian/sync/feature_limit_ui branch December 1, 2023 09:05
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.

2 participants