Skip to content

[BB2-1701] Node Sample App code cleanup and re-factor to use SDK#33

Merged
JFU-GIT merged 6 commits intomasterfrom
jfuqian/BB2-1701-Node-Sample-App-code-cleanup
Dec 5, 2022
Merged

[BB2-1701] Node Sample App code cleanup and re-factor to use SDK#33
JFU-GIT merged 6 commits intomasterfrom
jfuqian/BB2-1701-Node-Sample-App-code-cleanup

Conversation

@JFU-GIT
Copy link
Contributor

@JFU-GIT JFU-GIT commented Nov 22, 2022

JIRA Ticket:
BB2-1701

User Story or Bug Summary:

Description:

With the Blue Button API app SDK (Node JS) going to be published, it is highly desirable that BB2 Python Sample App be re-factored to use the SDK, which will greatly reduce the impl work of a BB2 API app and a great demo if the value of SDK.

AC:

BB2 Node JS Sample App code base refactored to use BB2 Node Js SDK

What Does This PR Do?

Refactor BB2 Node JS React Sample App code to use BB2 Node JS SDK for OAUTH2 Flow and FHIR Data Flow processing.

Note, the client component (the react front end) remain the same.
the server component code now only has index.ts (dozens lines of code only focus on application business logic) - a demonstration that the SDK greatly simplified the app development.

Removed unit tests - with the complex logic under the SDK, unit tests are not needed here any more, instead end to end tests (such as selenium tests - front end and back end involved) are more suitable guard the sample app from broken or regression

What Should Reviewers Watch For?

  1. Read the README.md and README-bb2-dev.md, make sure they are accurate
  2. Following the README.md/README-bb2-dev.md, going through node (npm/yarn) install,
  3. Check the Dockerfile for correctness, anything to improve?
  4. Run the sample app following the updated README.md, including copy and edit the configuration file properly to point to the registered apps at target ENV, remote BB2 target ENVs: PRODUCTION, SANDBOX
  5. Sample app run is docker compose based, please following the README to start the docker compose after properly configured
  6. After the sample app started (both client and server), interact with the front end : authorize, do the synthetic bene login and access grant / deny, and observed the result on the front end
  7. Lastly, also run the selenium tests (check README-bb2-dev.md for more details) - might need to clean up the residue containers, the followings are some docker commands to help to clean up residue containers:

delete images and cleanup dangling:

docker image ls | awk '{ print $3}' | grep -v "IMAGE" | xargs docker image rm -f
docker volume rm $(docker volume ls -qf dangling=true)

total wipe out

docker system prune -a --volumes

If you're reviewing this PR, please check these things, in particular:

  • TODO

Notes:

  1. unit tests (jest) is removed because by using SDK, sample app no longer have much deep logic on OAUTH2 flow implementation and FHIR resource IO processing, there is only one file index.ts and dozens of lines of source code that belongs to the sample app (server component).
    instead end to end testing - selenium tests added to cover the main scenarios
  2. currently selenium tests can be run at local dev environment, making selenium tests part of CI check workflow needs secured distribution of the oauth2 app creds (client_id, client_secret) which can be part of the later sample app repo CI check work.
  3. With this PR, BB2 Node JS SDK is consumed from a local tar ball since BB2 Node Js SDK has not been published, package.json needs fixed to install the SDK from npm after SDK publish.

@JFU-GIT JFU-GIT requested review from ajshred and dtisza1 November 25, 2022 17:57
Copy link

@ajshred ajshred 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 James!

Copy link
Contributor

@dtisza1 dtisza1 left a comment

Choose a reason for hiding this comment

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

@JFU-GIT This is looking good.

Just one small change request.

It tested OK for me locally.

README.md Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
cp server/sample-bluebutton-config.json -> server/.bluebutton-config.json
cp server/sample-bluebutton-config.json server/.bluebutton-config.json

Suggested change so that a copy & paste to the command line works.

Copy link
Contributor

@dtisza1 dtisza1 left a comment

Choose a reason for hiding this comment

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

@JFU-GIT This looks good to me!

Great work on this!

@JFU-GIT JFU-GIT merged commit abd27ee into master Dec 5, 2022
@JFU-GIT JFU-GIT deleted the jfuqian/BB2-1701-Node-Sample-App-code-cleanup branch December 5, 2022 15:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

Comments