Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Fix SYNC_CLEAR_HISTORY #6625

Merged
merged 2 commits into from
Jan 13, 2017
Merged

Conversation

ayumi
Copy link
Contributor

@ayumi ayumi commented Jan 12, 2017

Clearing history wasn't clearing SyncRecord historySites.
This happened because app/sync.js doAction() always checks if action.item is a proper Sync item.

This PR resolves it by checking for action.item only when the action requires it.

Test Plan:

  1. Sync should be enabled.
  2. Clear browsing history.
  3. In the console observe Deleting category: HISTORY_SITES

Auditors: @diracdeltas

@ayumi ayumi requested a review from diracdeltas January 12, 2017 23:40
@ayumi ayumi force-pushed the fix/syncing-clear-history branch from 6d17010 to cddf093 Compare January 13, 2017 00:08
@ayumi ayumi changed the base branch from feature/syncing to feature/syncing-0.13.1 January 13, 2017 00:09
syncConstants.SYNC_ADD_SITE_SETTING,
syncConstants.SYNC_UPDATE_SITE_SETTING,
syncConstants.SYNC_REMOVE_SITE_SETTING
]
Copy link
Member

Choose a reason for hiding this comment

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

it would be slightly easier for this to be SYNC_ACTIONS_WITHOUT_ITEMS since most actions in the future will probably have items

Copy link
Member

Choose a reason for hiding this comment

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

i think these constants should go inside validateAction since they are only used in there

@ayumi ayumi force-pushed the fix/syncing-clear-history branch from cddf093 to 4ef2256 Compare January 13, 2017 00:22
Copy link
Member

@diracdeltas diracdeltas left a comment

Choose a reason for hiding this comment

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

seems legit

@ayumi
Copy link
Contributor Author

ayumi commented Jan 13, 2017

No new failed tests.

@ayumi ayumi merged commit f83dd6e into feature/syncing-0.13.1 Jan 13, 2017
@ayumi ayumi deleted the fix/syncing-clear-history branch January 13, 2017 01:36
@luixxiul luixxiul added this to the 0.13.4 milestone Feb 15, 2017
@luixxiul luixxiul mentioned this pull request Feb 15, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants