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

Fixes bookmark move location cache #10819

Merged
merged 1 commit into from
Sep 15, 2017

Conversation

NejcZdovc
Copy link
Contributor

@NejcZdovc NejcZdovc commented Sep 6, 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.

Resolves #7032

Auditors:

Test Plan: specified in #7032 (comment)

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

@NejcZdovc NejcZdovc added this to the 0.21.x (Nightly Channel) milestone Sep 6, 2017
@NejcZdovc NejcZdovc self-assigned this Sep 6, 2017
@NejcZdovc
Copy link
Contributor Author

NejcZdovc commented Sep 6, 2017

unit tests will be added in #10310

Resolves brave#7032

Auditors:

Test Plan:
@codecov-io
Copy link

codecov-io commented Sep 6, 2017

Codecov Report

Merging #10819 into master will decrease coverage by 0.01%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master   #10819      +/-   ##
==========================================
- Coverage   54.17%   54.15%   -0.02%     
==========================================
  Files         247      247              
  Lines       21548    21553       +5     
  Branches     3338     3338              
==========================================
  Hits        11673    11673              
- Misses       9875     9880       +5
Flag Coverage Δ
#unittest 54.15% <0%> (-0.02%) ⬇️
Impacted Files Coverage Δ
app/common/state/bookmarksState.js 75% <0%> (-1.69%) ⬇️

Copy link
Contributor

@ayumi ayumi left a comment

Choose a reason for hiding this comment

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

I'm not sure how to repro the original problem, but I think the code looks good.

@luixxiul
Copy link
Contributor

@ayumi please see: #7032 (comment)

bug

@ghost ghost added the sprint/1 label Sep 13, 2017
@NejcZdovc NejcZdovc requested a review from ayumi September 14, 2017 08:29
@ayumi
Copy link
Contributor

ayumi commented Sep 14, 2017

@luixxiul the test plan seems to work on OSX on master?

STR is kind of confusing. First there are these two steps:

Make sure folder is selected as bookmark toolbar, instead of the new folder in which you put it

Bookmark toolbar remains location rather than the folder name

The steps don't seem to agree with: "Actual result: Root folder name is displayed, even if the bookmark exists under nested folder" because during STR the bookmark was specified as "bookmark toolbar".

I tried:
A) "Bookmark to top level / toolbar, create folder, move bookmark to folder, import"
B) "Create folder, bookmark and change dialog folder to folder, import"

Both seemed okay.

@luixxiul
Copy link
Contributor

CC @srirambv

Copy link
Contributor

@cezaraugusto cezaraugusto left a comment

Choose a reason for hiding this comment

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

++

@cezaraugusto cezaraugusto merged commit b47daf7 into brave:master Sep 15, 2017
@srirambv
Copy link
Collaborator

Worked fine on Windows

@bbondy bbondy modified the milestones: 0.21.x (Developer Channel), 0.20.x (Beta Channel) Oct 25, 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.

7 participants