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 25479
Build 50539: Build Diff
Build 50538: 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

@bot build-e.cash

teamcity awarded a token.Oct 31 2023, 05:02

@bot preview-e.cash

The build failed due to an unexpected infrastructure outage. The administrators have been notified to investigate. Sorry for the inconvenience.

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.

Looks good on the preview

web/e.cash/data/__tests__/scores.test.js
155	Is this change related to the score cutoff change?
web/e.cash/data/scores.js
154	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	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

edit comments, add scorethreshold as a parameter and update tests

Harbormaster completed remote builds in B25491: Diff 42880.Oct 31 2023, 16:39

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

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

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

add more robust unit test

Harbormaster completed remote builds in B25519: Diff 42922.Nov 2 2023, 16:32

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