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

More Sync sites refactor fixes and remove siteUtil.js #10222

Merged
merged 1 commit into from
Aug 2, 2017
Merged

More Sync sites refactor fixes and remove siteUtil.js #10222

merged 1 commit into from
Aug 2, 2017

Conversation

ayumi
Copy link
Contributor

@ayumi ayumi commented Aug 1, 2017

Submitter Checklist:

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Ran git rebase -i to squash commits (if needed).
  • Tagged reviewers and labelled the pull request as needed.

Test Plan:

Reviewer Checklist:

Tests

  • Adequate test coverage exists to prevent regressions
  • Tests should be independent and work correctly when run individually or as a suite ref
  • New files have MPL2 license header

@ayumi ayumi self-assigned this Aug 1, 2017
@ayumi ayumi requested a review from NejcZdovc August 1, 2017 08:21
NejcZdovc
NejcZdovc reviewed Aug 1, 2017
View reviewed changes
app/common/lib/historyUtil.js Outdated
}
const order = site.get('order')
const location = site.get('location')
const tags = site.get('tags')
Copy link
Contributor

@NejcZdovc NejcZdovc Aug 1, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Is this check added, because we still have tags property on other devices? tags were removed from browser-laptoprepo

Copy link
Contributor

@NejcZdovc NejcZdovc Aug 1, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

as an alternative we can add type property to the history object like we have it for bookmarks and bookmarks folders and use siteTags.HISTORY

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for syncing history sites, we actually don't need tags nor type. I thought it might be useful to have a reusable isHistorySite() but I think I'll just re-remove it from this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

sounds good to me, let's remove it

NejcZdovc
NejcZdovc reviewed Aug 1, 2017
View reviewed changes
test/unit/app/common/lib/historyUtilTest.js Outdated
it('returns false for a default site', function () {
const siteDetail = Immutable.fromJS({
location: testUrl1,
tags: [siteTags.DEFAULT],
Copy link
Contributor

Choose a reason for hiding this comment

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

siteTags.DEFAULT was deprecated

NejcZdovc
NejcZdovc reviewed Aug 1, 2017
View reviewed changes
test/unit/app/common/lib/historyUtilTest.js Outdated
it('returns false for a brave default site', function () {
const siteDetail = Immutable.fromJS({
location: testUrl1,
tags: [siteTags.DEFAULT],
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

@NejcZdovc NejcZdovc force-pushed the refactor/sites-split branch 2 times, most recently from f587994 to c7d84ec Compare August 1, 2017 19:42
@ayumi ayumi force-pushed the refactor/sites-split-sync-fix-2 branch 2 times, most recently from 3e1899b to 90f81e9 Compare August 1, 2017 22:07
@ayumi
Copy link
Contributor Author

ayumi commented Aug 1, 2017 •
edited
Loading

@NejcZdovc I've made updates. There are now only 2 failing tests:

npm run test -- --grep='^Syncing'

  1) Syncing then turning it off, then turning it on sends new records bookmark folder:
     Promise was rejected with the following reason: timeout
  Error

  2) Syncing then turning it off, then turning it on sends new records history:
     Error: Promise was rejected with the following reason: timeout
      at elements("table.sortableTable tbody tr") - brave.js:428:21

Trying those test plans manually seems to work, so I think it's an error in the tests.

@ayumi ayumi force-pushed the refactor/sites-split-sync-fix-2 branch 3 times, most recently from 4790fc0 to ddd0205 Compare August 1, 2017 23:51
@NejcZdovc
Copy link
Contributor

test 1 is passing now

@ayumi ayumi force-pushed the refactor/sites-split-sync-fix-2 branch from ddd0205 to 7990ccb Compare August 2, 2017 00:18
@NejcZdovc NejcZdovc merged commit 6ba8e96 into refactor/sites-split Aug 2, 2017
@ayumi ayumi deleted the refactor/sites-split-sync-fix-2 branch August 2, 2017 00:26
NejcZdovc pushed a commit that referenced this pull request Aug 2, 2017
@ayumi @NejcZdovc
More Sync sites refactor fixes and remove siteUtil.js (#10222)
cf0241b
NejcZdovc pushed a commit that referenced this pull request Aug 3, 2017
@ayumi @NejcZdovc
More Sync sites refactor fixes and remove siteUtil.js (#10222)
6955ca1
NejcZdovc pushed a commit that referenced this pull request Aug 4, 2017
@ayumi @NejcZdovc
More Sync sites refactor fixes and remove siteUtil.js (#10222)
1c249e6
@ayumi ayumi mentioned this pull request Aug 8, 2017
Closed
dfperry5 pushed a commit to dfperry5/browser-laptop that referenced this pull request Aug 18, 2017
@ayumi @dfperry5
More Sync sites refactor fixes and remove siteUtil.js (brave#10222)
ff2f0c9
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@NejcZdovc NejcZdovc NejcZdovc left review comments

Assignees

@ayumi ayumi

Labels
feature/sync
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ayumi @NejcZdovc