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

Moves bookmarks manager from js to app #10074

Merged
merged 1 commit into from
Jul 21, 2017

Conversation

NejcZdovc
Copy link
Contributor

@NejcZdovc NejcZdovc commented Jul 20, 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 #10075

Auditors: @bsclifton

Test Plan:

  • check if bookmarks manager is displayed correctly

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.20.x (Developer Channel) milestone Jul 20, 2017
@NejcZdovc NejcZdovc self-assigned this Jul 20, 2017
NejcZdovc added a commit to NejcZdovc/browser-laptop that referenced this pull request Jul 20, 2017
Resolves brave#10074

Auditors: @bsclifton

Test Plan:
- check if bookmarks manager is displayed correctly
Resolves brave#10075

Auditors: @bsclifton

Test Plan:
- check if bookmarks manager is displayed correctly
@NejcZdovc
Copy link
Contributor Author

@cezaraugusto I am gonna merge this one, because it's a simple split and move. If you find anything that is problematic, please let me know and I will do a follow up. I need this one in the master to continue with my work on #9978

@NejcZdovc NejcZdovc merged commit 3a5bc56 into brave:master Jul 21, 2017
@cezaraugusto
Copy link
Contributor

well I was reviewing it now, pls ping me before doing that

@NejcZdovc
Copy link
Contributor Author

NejcZdovc commented Jul 21, 2017

@cezaraugusto it was 3am+ in the morning where you are, so I was assuming that you are in the bed already 😃

@luixxiul
Copy link
Contributor

@NejcZdovc Unless test code is updated/added, please avoid adding QA/no-qa-needed to closed issues, this case #10075. thanks

@NejcZdovc
Copy link
Contributor Author

@luixxiul test code for bookmark manager is already there, this PR just moves files into app folder, no logic was changed, no new code was added.

@luixxiul
Copy link
Contributor

luixxiul commented Jul 22, 2017

It is possible for all of us to make a mistake, however tiny it can be, even when moving something to somewhere, and I believe that is partly why we stopped accepting a direct merge to the master branch and started requring a review before merging a PR. IMO manual tests are required on the same reason

@bsclifton bsclifton changed the title Moves bookarks manager from js to app Moves bookmarks manager from js to app Jul 23, 2017
@bsclifton
Copy link
Member

bsclifton commented Jul 23, 2017

I agree- each pull request needs to do one of the following:

  • Have manual test steps specified
    OR
  • Be covered with automated tests

Reviewing this PR, these changes should be covered by automated tests. In this case, we can call out the specific statements to run... For example:
npm run test -- --grep="about:bookmarks"

@NejcZdovc
Copy link
Contributor Author

With this PR I added label QA/no-qa-needed because of the following things:

  1. bookmark manager is covered via existing automated tests
  2. no new code was added
  3. this PR is a base PR for some of the work I had done in Split sites into separate entities. #9978 which is landing in the same milestone as this PR, so bookmark manager will be tested really extensively in Split sites into separate entities. #9978 (I could do this inside Split sites into separate entities. #9978, but I rather split moves into separate PR's so that we can track the changes that are then done in real PR)

Hope that this make sense and I wanted to save some QA time with it 😃

@luixxiul
Copy link
Contributor

if adding QA/no-qa-needed, at least let's wait a code review is done. Otherwise it would look too hasty, IMHO.

Without landing the commit to the master, you could have been able to continue work #9978 by cherry-picking the commit temporarily.

@NejcZdovc
Copy link
Contributor Author

@luixxiul I already talked with @cezaraugusto about this review and that was completely my mistake, because I though that he was asleep, but he was actually already reviewing it 😃

I see that this is important, so I will leave QA needed there for this type of PR's (moves and tests). IMHO this will just take additional (valuable) time for things that don't needed QA attention from QA team

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.

4 participants