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

feat: Adds display name attribute to subsidy access policy creation API #365

Merged
merged 2 commits into from
Nov 27, 2023

Conversation

brobro10000
Copy link
Member

@brobro10000 brobro10000 commented Nov 27, 2023

Adds display_name to the subsidy access policy POST method and related data transformation functions.

Copy link

codecov bot commented Nov 27, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (438844e) 88.41% compared to head (09f4e26) 88.41%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #365   +/-   ##
=======================================
  Coverage   88.41%   88.41%           
=======================================
  Files         161      161           
  Lines        3417     3417           
  Branches      841      841           
=======================================
  Hits         3021     3021           
  Misses        393      393           
  Partials        3        3           

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

@brobro10000 brobro10000 marked this pull request as ready for review November 27, 2023 16:51
@@ -503,6 +504,7 @@ export function transformPolicyData(formData, catalogCreationResponse, subsidyCr
|| subsidyCreationResponse.length === 0
) { return []; }
const payloads = policies.map((policy, index) => ({
displayName: policy.accountName,
Copy link
Member

Choose a reason for hiding this comment

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

[nit, question, non-blocking] What does account refer to here? I'm wondering if it might make sense to more closely align the property names to how the API returns these attributes? That said, if we take any action here, definitely for a separate PR given unrelated variable names, etc. would also need to change.

For example, at first glance, it's not immediately obvious to me that accountName is referring to the display name of a budget; initially, I had read "account" to mean mean the enterprise customer, not the budget itself (e.g., is policy.accountName the enterprise customer name?).

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed on the sentiment for the renaming of the object attributes. I know theres an upcoming PR that revamps some of the naming scheme to be more understandable, so it was intentional to keep this PR as simple as possible.

But I will definitely leave this comment open as a reference to when we do have to take on that future work. 👍🏽

@brobro10000 brobro10000 merged commit 895ac85 into master Nov 27, 2023
5 checks passed
@brobro10000 brobro10000 deleted the hu/ent-7987 branch November 27, 2023 17:41
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