Page MenuHomePhabricator
Differential D14707

[e.cash] Filter out low exchange scores
ClosedPublic
Actions

Authored by johnkuney on Oct 31 2023, 04:59.

Details

Reviewers
Fabien
Group Reviewers
Restricted Project
Commits
rABC3142a9c245dc: [e.cash] Filter out low exchange scores
Summary

As discussed adding some logic to scores.js to filter out scores below a certain threshold (60 for now)
Updated the test data to reflect this and modified a test so it would pass

Test Plan

Preview the site and check items on the get-ecash page with low scores are gone
check all test pass - npm run test

Diff Detail

Repository
rABC Bitcoin ABC
Branch
ecash-getscores-modifications
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 25519
Build 50619: Build Diff
Build 50618: arc lint + arc unit

Event Timeline

johnkuney created this revision.Oct 31 2023, 04:59
Herald added a reviewer: Restricted Project. · View Herald TranscriptOct 31 2023, 04:59
johnkuney requested review of this revision.Oct 31 2023, 04:59
Harbormaster completed remote builds in B25479: Diff 42865.Oct 31 2023, 05:00
Mengerian added a subscriber: Mengerian.Oct 31 2023, 05:02

@bot build-e.cash

teamcity awarded a token.Oct 31 2023, 05:02
Mengerian added a comment.Oct 31 2023, 05:03

@bot preview-e.cash

teamcity updated a token.Oct 31 2023, 05:03
The build failed due to an unexpected infrastructure outage. The administrators have been notified to investigate. Sorry for the inconvenience.
teamcity added a comment.Oct 31 2023, 05:04

Build Bitcoin ABC Diffs / Diff Testing (preview-e.cash) passed.
Preview is available at http://54.39.104.128:41686 for the next 60 minutes.

Mengerian added a comment.Oct 31 2023, 05:27

Looks good on the preview

web/e.cash/data/__tests__/scores.test.js
155 ↗(On Diff #42865)

Is this change related to the score cutoff change?

web/e.cash/data/scores.js
154 ↗(On Diff #42865)

Maybe it's just me, but the comment seems a bit confusing.

I see that's how it's implemented, but the comment doesn't make it obvious that score is the primary sorting, alphabetically only factors in when the other stuff is equal.

Fabien requested changes to this revision.Oct 31 2023, 14:21
Fabien added a subscriber: Fabien.
Fabien added inline comments.
web/e.cash/data/scores.js
158 ↗(On Diff #42865)

There is no test for this sort function. You certainly want to move the threshold to some config file, pass it as a parameter to this sort function and add a unit test to check the behavior.

This revision now requires changes to proceed.Oct 31 2023, 14:21
johnkuney updated this revision to Diff 42880.Oct 31 2023, 16:39

edit comments, add scorethreshold as a parameter and update tests

Harbormaster completed remote builds in B25491: Diff 42880.Oct 31 2023, 16:39
johnkuney added a comment.Oct 31 2023, 16:42

Okay added, I couldnt think of a better place to put the scoreThreshold than at the top of scores.js, but open to suggestions

johnkuney added a comment.Oct 31 2023, 16:44

And as far as that other test change in getScoreCardData, I added it here because the sort change was causing that test to fail as it was

Fabien requested changes to this revision.Nov 2 2023, 11:16

The unit test needs to give confidence the code is working, not only testing the happy path (as per telegram discussion)

This revision now requires changes to proceed.Nov 2 2023, 11:16
johnkuney updated this revision to Diff 42922.Nov 2 2023, 16:32

add more robust unit test

Harbormaster completed remote builds in B25519: Diff 42922.Nov 2 2023, 16:32
Fabien accepted this revision.Nov 3 2023, 13:51

Very nice ! That is a great unit tests which is easy to read, understand and review.

This revision is now accepted and ready to land.Nov 3 2023, 13:51
Closed by commit rABC3142a9c245dc: [e.cash] Filter out low exchange scores (authored by johnkuney). · Explain WhyNov 3 2023, 14:35
This revision was automatically updated to reflect the committed changes.
johnkuney added a commit: rABC3142a9c245dc: [e.cash] Filter out low exchange scores.

Revision Contents
Changeset List

PathSize
web/
e.cash/
data/
__mocks__/
__tests__/
80 lines
25 lines

Diff 42922

View Options

web/e.cash/data/__mocks__/scoresMock.js

Loading...
View Options

web/e.cash/data/__tests__/scores.test.js

Loading...
View Options

web/e.cash/data/scores.js

Loading...