Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DDW-360] missing v1 api endpoints integration 6-9 #1079

Conversation

MarcusHurney
Copy link
Contributor

@MarcusHurney MarcusHurney commented Sep 4, 2018

This PR implements V1 endpoints for the follow Ada API requests:

6.) nextAdaUpdate.js

  • Contains a check in request.js for a 404 response which indicates an update is not available instead of an error.

7.) applyAdaUpdate.js
8.) postponeAdaUpdate.js
9.) adaTestReset.js

Review Checklist:

Basics

  • PR is updated to the most recent version of target branch (and there are no conflicts)
  • PR has good description that summarizes all changes and shows some screenshots or animated GIFs of important UI changes
  • CHANGELOG entry has been added and is linked to the correct PR on GitHub
  • Automated tests: All acceptance tests are passing (yarn run test)
  • Manual tests (minimum tests should cover newly added feature/fix): App works correctly in development build (yarn run dev)
  • Manual tests (minimum tests should cover newly added feature/fix): App works correctly in production build (yarn run package / CI builds)
  • There are no flow errors or warnings (yarn run flow:test)
  • There are no lint errors or warnings (yarn run lint)
  • Text changes are proofread and approved (Jane Wild)
  • There are no missing translations (running yarn run manage:translations produces no changes)
  • UI changes look good in all themes (Alexander Rukin)
  • Storybook works and no stories are broken (yarn run storybook)
  • In case of dependency changes yarn.lock file is updated

Code Quality

  • Important parts of the code are properly documented and commented
  • Code is properly typed with flow
  • React components are split-up enough to avoid unnecessary re-rendering
  • Any code that only works in Electron is neatly separated from components

Testing

  • New feature / change is covered by acceptance tests
  • All existing acceptance tests are still up-to-date
  • New feature / change is covered by Daedalus Testing scenario
  • All existing Daedalus Testing scenarios are still up-to-date

After Review:

  • Merge PR
  • Delete source branch
  • Move ticket to done on the Youtrack board

}`;
} else if (statusCode === 400) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@MarcusHurney are you sure the expected statusCode is 400?
In the docs it says it should be 404 - AKA not-found?
404

Also, I am a bit worried that any other status code 400/404 will not be properly handled with this if clause. E.g. other endpoints might have status 400 as real error?

@nikolaglumac
Copy link
Contributor

@MarcusHurney the reason this endpoint was returning error code 400 is the fact you are running the backend against the old data layer. I have tested it against the new data layer and I got the expected 404 error code. Since we will switch to the new data layer I have updated the code: ffe3538
Will explain to you how to run the backend in the same way as I do ;)

@nikolaglumac
Copy link
Contributor

@MarcusHurney V1 Api endpoint /api/internal/reset-wallet-state is still not implemented on the backend side. This means we are unable to run tests with the new data layer.

@nikolaglumac nikolaglumac reopened this Sep 5, 2018
@nikolaglumac
Copy link
Contributor

@MarcusHurney I have tested the next-update endpoint and it works correctly 👍

@MarcusHurney
Copy link
Contributor Author

MarcusHurney commented Sep 5, 2018

@nikolaglumac
Thanks for looking into this! I like this check here because the path shouldn't change:
options.path === '/api/internal/next-update'. Combine that with the response's status code and it should be safer than checking for messages (strings) as truthy values. I know we love Javascript but we can't leave it alone with our sisters!

@MarcusHurney MarcusHurney removed the WIP label Sep 6, 2018
@nikolaglumac nikolaglumac merged commit 3ca61ad into feature/ddw-321-v1-api-integration Sep 6, 2018
@nikolaglumac nikolaglumac deleted the feature/ddw-360-missing-v1-api-endpoints-integration-6-9 branch September 6, 2018 13:35
This was referenced Oct 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants