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

BLAIS5-4190 Add cma serverpark access to cma roles #352

Merged

Conversation

am-ons
Copy link
Contributor

@am-ons am-ons commented Jun 17, 2024

🛠 Changes being made

Here give examples of the changes you've made in this pull request. Include an itemized list if you can. It'll help the reviewer

✨ What's the context?

What's the context for the changes?

🧠 Rationale behind the change

Why did you choose to make these changes? Were there any trade-offs you had to consider?

  • Added the code and using blaise-api-node-client v1.1.0 rather then using branch BLAIS5-4140

🧪 Test plan

How do you know the changes are safe to ship to production?

  • Tested on local dev box and el48 sandbox env
  • The code coverage is below the required level, and team agreed to proceed with this ticket to go live and address the test coverage as a separate high priority ticket. That's why this ticket is now in peer review without sufficient test coverage.

📸 Screenshots (optional)

If you made UI changes, what are the before an afters?

🏎 Quality check

  • Are your changes following SOLID principles?
  • Are there any erroneous console logs, debuggers or leftover code in your changes?
  • Walk away, take a break, re-read what you filled out above does it make sense if you were coming in cold? What extra context could you provide?

@am-ons am-ons force-pushed the feature/BLAIS5-4190-Add-CMA-Serverpark-Access-To-CMA-Roles branch 2 times, most recently from f2fabf7 to 13af414 Compare June 17, 2024 10:33
…lt Server Park" to "Server Parks"

    ClientConfig reads RoleToServerParksMap from json file
    Read role_to_serverparks_map_json from json file
    Rename RoleToServerParksMap to RoleToServerParksMap
    Server side RoleToServerParksDict config change
    Client side RoleToServerParksDict config changes

fix: Add resolveJsonModule to tsconfig.server.json
@am-ons am-ons force-pushed the feature/BLAIS5-4190-Add-CMA-Serverpark-Access-To-CMA-Roles branch from 13af414 to ec9e152 Compare June 17, 2024 10:41
Copy link

codecov bot commented Jun 17, 2024

Codecov Report

Attention: Patch coverage is 9.09091% with 20 lines in your changes missing coverage. Please review.

Project coverage is 46.28%. Comparing base (b154979) to head (81618e8).

Current head 81618e8 differs from pull request most recent head 738bf98

Please upload reports for the commit 738bf98 to get more accurate results.

Files Patch % Lines
src/pages/users/NewUser.tsx 0.00% 9 Missing ⚠️
server/BlaiseAPI/index.ts 0.00% 8 Missing ⚠️
src/ClientConfig.ts 0.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #352      +/-   ##
==========================================
- Coverage   47.50%   46.28%   -1.22%     
==========================================
  Files          29       30       +1     
  Lines         640      659      +19     
  Branches      134      138       +4     
==========================================
+ Hits          304      305       +1     
- Misses        336      354      +18     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@am-ons am-ons requested a review from a team June 17, 2024 14:40
…ed in separate ticket for adding more tests.
@am-ons am-ons marked this pull request as ready for review June 18, 2024 15:39
Copy link
Contributor

@elthorne elthorne left a comment

Choose a reason for hiding this comment

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

Beautiful, clean code and works as expected.

@elthorne elthorne merged commit 7b45d57 into main Jun 19, 2024
4 checks passed
@elthorne elthorne deleted the feature/BLAIS5-4190-Add-CMA-Serverpark-Access-To-CMA-Roles branch June 19, 2024 11:07
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.

2 participants