Page MenuHomePhabricator

[e.cash] Fetch exchanges and services and render on get ecash page
ClosedPublic

Authored by johnkuney on Jun 9 2023, 05:58.

Details

Summary

Adding the list of exchanges and services to the get ecash page. The data is being pulled from the scorecard.cash api using the recently added methods, and sorted on the same scoring criteria that scorecard.cash uses so better services are listed first

Test Plan

preview the site and go to the get ecash page
check everything looks good and all the links work

Diff Detail

Repository
rABC Bitcoin ABC
Branch
get-ecash-content
Lint
Lint Warnings
SeverityLocationCodeMessage
Warningweb/e.cash/public/animations/exchanges-coin-flip.json:47SPELL2Possible Spelling Mistake
Warningweb/e.cash/public/animations/exchanges-coin-flip.json:79SPELL2Possible Spelling Mistake
Warningweb/e.cash/public/animations/exchanges-coin-flip.json:151SPELL2Possible Spelling Mistake
Warningweb/e.cash/public/animations/exchanges-coin-flip.json:183SPELL2Possible Spelling Mistake
Warningweb/e.cash/public/animations/exchanges-coin-flip.json:207SPELL1Possible Spelling Mistake
Unit
No Test Coverage
Build Status
Buildable 23986
Build 47582: Build Diff
Build 47581: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
This revision now requires changes to proceed.Jun 10 2023, 00:03
Fabien requested changes to this revision.Jun 10 2023, 07:33
Fabien added inline comments.
web/e.cash/utils/pageUtils.js
2 ↗(On Diff #40716)

This is not related to pages.
Also there is no point calling the file utils/pageUtils.js. If it's in utils, it should be obvious enough with utils/page.js that this is an util.

In any case you should really think twice before creating a util/ directory. It's a smell of a bad design, 99% of the time you can find a better place for the code. Like, score.js ? Next to scoringCriteria.js ? And why not making it a single file so it's self contained and can be tested easily ?

improve testing and comments, rename and move utils function

Good suggestions in general, added.

As for the point about the api failing, getStaticProps only runs at build time, and if the call fails it will abort the build and throw an error, so the getScores function wont be called

bytesofman added inline comments.
web/e.cash/data/__tests__/scores.test.js
39 ↗(On Diff #40743)

kinda confusing description

'should leave input array unchanged if length is divisible by 3'

web/e.cash/data/scores.js
106 ↗(On Diff #40743)
107 ↗(On Diff #40743)

would be nice to know this only runs on app build and not on page load

web/e.cash/pages/get-ecash.js
112 ↗(On Diff #40743)
  1. Should have some comments about what this function is doing, when it runs, etc
  1. this function should also be unit tested. it's not clear that the error will be thrown on an API failure. What comes from the API when it's down?

For this unit test, you will want to mock fetch , i.e. one test case where you mock expected responses, another where you mock error responses and show that the error is thrown

This revision now requires changes to proceed.Jun 12 2023, 20:00
web/e.cash/pages/get-ecash.js
112 ↗(On Diff #40743)

any other guidance/ideas for testing getstaticprops? Having trouble finding good docs on mocking fetch, and Im not sure if testing getstatic props will behave the same as a regular function since its part of the next.js library...

web/e.cash/pages/get-ecash.js
112 ↗(On Diff #40743)

options

  1. https://www.npmjs.com/package/jest-fetch-mock
  2. https://jestjs.io/docs/mock-functions

Cashtab uses (2)
ecash-herald and alias-server use axios and an npm dependency for mocking axios, like (1) above

improve notes, still need to write test for getstaticprops

move the scorecard api call outside of getstatic props and make a test for it

bytesofman added inline comments.
web/e.cash/data/scores.js
176 ↗(On Diff #40767)

if this is only used in this function, just hardcode it into the function or define it at the top of the function

defining it outside here creates an unecessary global variable

184 ↗(On Diff #40767)

what does responses look like here in the event of an error?

We have a unit test which is passing, but this is still a weird way to handle it. more robust would be

let responses;
try {
responses = await Promise.all([..])
} catch (err) {
throw new Error(`Failed to fetch scorecard API response`);
}
190 ↗(On Diff #40767)

getScores requires the data to be a very specific shape, e.g. have a .criteria key etc.

No error handling for this

Possible for the API server DB to fail and return an empty array or object

Should have some kind of validation and throw an error if the data is not the shape you need

This revision now requires changes to proceed.Jun 13 2023, 19:00

add test for empty array or object response

bytesofman added inline comments.
web/e.cash/data/scores.js
190 ↗(On Diff #40772)

this isn't the approach we want to take here. While this would work on the very specific possible responses of an empty array or empty object, what we want to show is that an error is thrown on any response that is not of the required shape.

It doesn't need to be too complicated to get through this, though.

instead of return props: {exchanges: makeDivisibleByThree(getScores..}

just use the same try...catch block you already have to wrap the API call

try {

// Make API calls
const responses = apiCallsMadeSuccessfully

// Try to determine props from the api result
const props = {
            exchanges: makeDivisibleByThree(
                getScores(responses[0], exchangeScoringCriteria),
            ),
            instantExchanges: makeDivisibleByThree(
                getScores(responses[1], instantExchangeScoringCriteria),
            ),
            services: makeDivisibleByThree(
                getScores(responses[2], servicesScoringCriteria),
            ),
        }
} catch (err) {
throw err;
}
This revision now requires changes to proceed.Jun 13 2023, 21:26
bytesofman added inline comments.
web/e.cash/data/__tests__/scores.test.js
25 ↗(On Diff #40775)

description should not be identical to the one above

If they really are the same test, can move the logic of this one into the above one

32 ↗(On Diff #40775)

ditto

web/e.cash/pages/get-ecash.js
107–111 ↗(On Diff #40775)
This revision now requires changes to proceed.Jun 13 2023, 22:04

add comment and consolidate scoring test

Fabien requested changes to this revision.Jun 14 2023, 12:50

This diff is doing too much things at the same time. You should first add the score.js file and functions with its tests, then add the exchanges to the website.

web/e.cash/data/__mocks__/scoresMock.js
1 ↗(On Diff #40776)

I suppose it's generated ? If so please add a comment with instructions to regenerate and add a @generated mark

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

The header is missing, but I think this is the case for most if not all the files in the repo.
@bytesofman can you please look at adding linter support for the repo and make sure we don't lose time reviewing this kind of stuff ?

web/e.cash/data/scores.js
13 ↗(On Diff #40776)

Use proper doxygen layout:

/**
 * My wonderful comment.
 * Could very well span on multiple lines.
 */
111 ↗(On Diff #40776)

Remove or the doc won't link to the function

158 ↗(On Diff #40776)

dito, please go over the whole file

This revision now requires changes to proceed.Jun 14 2023, 12:50
johnkuney edited the test plan for this revision. (Show Details)
web/e.cash/next.config.js
6 ↗(On Diff #40822)

where is this being used?

10 ↗(On Diff #40822)

url is still hardcoded in get-ecash.js -- is this a nextjs thing? what breaks if this is removed?

web/e.cash/next.config.js
6 ↗(On Diff #40822)

its a next js thing for images that have an outside url. have to specify it here otherwise get this error

Screen Shot 2023-06-15 at 2.20.23 PM.png (860×2 px, 217 KB)

https://nextjs.org/docs/messages/next-image-unconfigured-host

web/e.cash/pages/get-ecash.js
111 ↗(On Diff #40822)

remove line break in between function comments and function

web/e.cash/next.config.js
12 ↗(On Diff #40827)

Is that the whole CORS config or does the server need to handle this as well ? If so you want to add it to the nginx.conf

web/e.cash/next.config.js
12 ↗(On Diff #40827)

afaik thats all we need here. It ensures the next.js image optimization API isn't exploited

Fabien added inline comments.
web/e.cash/next.config.js
12 ↗(On Diff #40827)

OK let's move on for now and we'll figure that out as needed

This revision is now accepted and ready to land.Jun 16 2023, 19:13