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

Mobile: Resolves #10245: Allow marking items as "ignored" in sync status #10261

Conversation

personalizedrefrigerator
Copy link
Collaborator

Summary

Adds an "ignore" button to the mobile sync status screen.

Resolves #10245.

Notes

  • At present, this "ignore" button is only shown on mobile. On desktop, there are currently more options to resolve a sync issue (e.g. by deleting a large item through the attachments screen).
  • An "ignore" button is currently not shown for resource download failures.
  • This pull request adds a sync_warning_dismissed field to sync_items, which requires upgrading the database.
    • If an item has sync_warning_dismissed = 1, that item will not cause a "some items cannot be synchronized" warning to be shown (other items can still trigger the warning).
    • An item with sync_warning_dismissed = 1 will still cause failures when attempting to switch sync targets.
      • Note: Particularly for errors like "exceeds maximum item size of 100 kB", it could make sense to still allow switching sync targets.
  • The sync status screen is migrated to TypeScript.

Screenshot

screenshot: In addition to the existing "retry" buttons to the right of each item in the "sync failed" list, items have an "ignore" button

Testing

Although this pull request has an automated test, it should also be tested manually. This can be done by:

  1. Setting up Joplin Server sync.
  2. Setting the maximum item size to a small number like 1024 (bytes)
    screenshot: Maximum item size set to 1024 in the Joplin Server config page
  3. Creating 1-2 items that are larger than this limit and syncing.
  4. Clicking on the "some items could not be synchronized" banner.
  5. Clicking "ignore" by each of the sync issue records.
  6. Verifying that the "some items could not be synchronized" error message disappears.
  7. Verifying that the item is moved under "ignored items"
  8. Resyncing
  9. Verifying that no sync warning is shown
  10. Opening sync status (configuration > tools > sync status)
  11. Clicking "retry" next to one of the ignored items
  12. Resyncing
  13. Verifying that the warning is shown again

This has been tested successfully on an Android 12 emulator.

Copy link
Owner

@laurent22 laurent22 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 pull request, that looks good.

Don't make ignored items prevent switching sync targets. Note that this solution could be okay for upload issues (e.g. "item too large to upload"), but could cause problems for if the item was disabled due to a download issue.

Yes let's do this for now.

By the way would it be possible to add a gap between the Retry and Ignore buttons?

}
expect(ignoredItemCount).toBe(noteCount);
});
});
Copy link
Owner

Choose a reason for hiding this comment

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

Thanks for adding these tests! I can't believe we never had tests for this service

@personalizedrefrigerator
Copy link
Collaborator Author

personalizedrefrigerator commented Apr 4, 2024

By the way would it be possible to add a gap between the Retry and Ignore buttons?

Updated screenshot:

screenshot: Ignore and retry buttons have several pixels of space between them

}

class StatusScreenComponent extends BaseScreenComponent<Props, State> {
public static navigationOptions(): any {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Based on the name, this seems to be related to React Navigation v1 (and should thus be safe to remove).

Edit: Removed in 7f530a9

Copy link
Owner

Choose a reason for hiding this comment

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

Yes probably. That was added a long time ago and never removed since I wasn't sure if it did something or not.


export default (): (SqlQuery|string)[] => {
return [
'ALTER TABLE sync_items ADD COLUMN sync_warning_dismissed INT NOT NULL DEFAULT "0"',
Copy link
Owner

Choose a reason for hiding this comment

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

You use the term "ignored" in some places and "dismissed" in other places. Would it be possible to use the same term everywhere?

@laurent22
Copy link
Owner

Looks good now, thanks!

@laurent22 laurent22 merged commit 03c3fee into laurent22:dev Apr 8, 2024
10 checks passed
@dud1337
Copy link

dud1337 commented Apr 20, 2024

Thank you for this!

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.

Mobile: Add a way to ignore sync issues related to a specific attachment
3 participants