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

Fixes Issue #10752, allows for sorting by publisher verification #12750

Merged
merged 2 commits into from
Feb 7, 2018
Merged

Fixes Issue #10752, allows for sorting by publisher verification #12750

merged 2 commits into from
Feb 7, 2018

Conversation

ryanml
Copy link
Contributor

@ryanml ryanml commented Jan 20, 2018

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.
  • Request a security/privacy review as needed. (Ask a Brave employee to help if you cannot access this document.)

Resolves #10752

Test Plan:
This can be done through the UI.

  1. Navigate to about:preferences#payments
  2. Click on the first column header to sort by publisher verification status
  3. Ensure that the ordering toggles for verified publishers (ASC/DESC)

Reviewer Checklist:

  • Request a security/privacy review as needed if one was not already requested.

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

This is a very simple fix for issue #10752. The SortableTable component looks to the 'value' property of a row object as its basis for sorting (We should mimic the configuration for the included row object, where the value property is either set to 1 or 0, depending on whether its turned on). Now the Verified Publisher column sorts as expected. Thanks!

@NejcZdovc NejcZdovc self-requested a review January 20, 2018 07:53
@NejcZdovc NejcZdovc added this to the 0.20.x Hotfix 1 milestone Jan 20, 2018
@codecov-io
Copy link

codecov-io commented Jan 20, 2018

Codecov Report

Merging #12750 into master will decrease coverage by 0.03%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #12750      +/-   ##
==========================================
- Coverage   56.09%   56.06%   -0.04%     
==========================================
  Files         279      279              
  Lines       27302    27303       +1     
  Branches     4441     4442       +1     
==========================================
- Hits        15316    15308       -8     
- Misses      11986    11995       +9
Flag Coverage Δ
#unittest 56.06% <ø> (-0.04%) ⬇️
Impacted Files Coverage Δ
...erer/components/preferences/payment/ledgerTable.js 87.81% <ø> (+0.06%) ⬆️
js/stores/appStoreRenderer.js 91.17% <0%> (-8.83%) ⬇️
app/renderer/components/reduxComponent.js 84.61% <0%> (-6.16%) ⬇️
js/stores/windowStore.js 27.25% <0%> (-0.31%) ⬇️

@ryanml
Copy link
Contributor Author

ryanml commented Jan 20, 2018

@NejcZdovc thanks for taking a look at this! :)

@ryanml
Copy link
Contributor Author

ryanml commented Jan 21, 2018

@NejcZdovc there is one possible improvment I can make to this PR (It serves more of a visual purpose than a functional one)

Currently, if a publisher is verified and included, the verification icon is green, if a publisher is verified and not included, the verification icon is not filled and is gray.

The fix in this PR is technically functionally satisfying, as it does sort by whether a publisher is verified or not, but it does look a little odd if the non included publishers are staggered throughout the ordering.

If you feel that it would beneficial for inclusion to factor in to the ordering as well, I can adjust the ternary in that value property to:

verified ? (this.enabledForSite(synopsis) ? 2 : 1) : 0

This will assign a value of 2 to verified and included publishers, a value of 1 to verified but not included publishers, and 0 for neither verified nor included. (I think this ends up looking a little nicer :)

Let me know your thoughts and I can push this in a commit to this PR.

screen shot 2018-01-21 at 2 39 44 pm

@NejcZdovc
Copy link
Contributor

@ryanml I like the idea, go for it

@ryanml
Copy link
Contributor Author

ryanml commented Jan 22, 2018

@NejcZdovc I've pushed the improvement :)

Copy link
Contributor

@NejcZdovc NejcZdovc left a comment

Choose a reason for hiding this comment

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

Looks good to me. Tested it out and it's working perfectly. Congrats on you first PR 🎉 Will merge it as soon as 0.20 is released

@ryanml
Copy link
Contributor Author

ryanml commented Jan 22, 2018

@NejcZdovc awesome! :) Thanks so much

@ryanml
Copy link
Contributor Author

ryanml commented Jan 31, 2018

@NejcZdovc was 0.20 released? :)

@NejcZdovc
Copy link
Contributor

@ryanml it was 😃

@ryanml
Copy link
Contributor Author

ryanml commented Jan 31, 2018

@NejcZdovc great! Excited to see this fix in place, I'm working on another as we speak :)

@alexwykoff alexwykoff modified the milestones: 0.20.x Hotfix 3 (Ledger improvments), 0.21.x (Beta Channel) Feb 6, 2018
@NejcZdovc NejcZdovc merged commit fd95339 into brave:master Feb 7, 2018
NejcZdovc added a commit that referenced this pull request Feb 7, 2018
…sort

Fixes Issue #10752, allows for sorting by publisher verification
NejcZdovc added a commit that referenced this pull request Feb 7, 2018
…sort

Fixes Issue #10752, allows for sorting by publisher verification
@NejcZdovc
Copy link
Contributor

master fd95339
0.22 2d14fb3
0.21 d3f8fd1

@NejcZdovc NejcZdovc modified the milestones: 0.21.x (Beta Channel), 0.21.x (Twitch) Feb 24, 2018
@NejcZdovc
Copy link
Contributor

NejcZdovc commented Feb 24, 2018

0.21 twitch e0670fd

NejcZdovc added a commit that referenced this pull request Feb 24, 2018
…sort

Fixes Issue #10752, allows for sorting by publisher verification
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.

cannot sort by publisher verification status in about:preferences#payments
4 participants