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

billing: marketplace UI (PROJQUAY-6551) #2595

Merged
merged 3 commits into from
Jan 11, 2024

Conversation

Marcusk19
Copy link
Contributor

adds UI in billing section for managing user and org-bound skus
add new skus for bulk purchase orders
change api endpoints to allow bulk attaching skus to orgs

adds UI in billing section for managing user and org-bound skus

add more unit tests for org binding

change endpoint for bulk attaching skus to orgs
Copy link

codecov bot commented Jan 9, 2024

Codecov Report

Attention: 10 lines in your changes are missing coverage. Please review.

Comparison is base (2645176) 70.63% compared to head (3c3a271) 70.67%.
Report is 1 commits behind head on master.

Files Patch % Lines
endpoints/api/billing.py 82.60% 5 Missing and 3 partials ⚠️
data/billing.py 33.33% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2595      +/-   ##
==========================================
+ Coverage   70.63%   70.67%   +0.03%     
==========================================
  Files         434      434              
  Lines       39887    39919      +32     
  Branches     5173     5180       +7     
==========================================
+ Hits        28174    28212      +38     
+ Misses      10088    10079       -9     
- Partials     1625     1628       +3     
Flag Coverage Δ
unit 70.67% <79.59%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@Marcusk19 Marcusk19 requested a review from kleesc January 9, 2024 19:51
@@ -1019,7 +1058,7 @@ def delete(self, orgname, subscription_id):
return ("Organization not valid", 400)

model.organization_skus.remove_subscription_from_org(organization.id, subscription_id)
return ("Deleted", 204)
return ("Deleted", 200)
Copy link
Member

Choose a reason for hiding this comment

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

Any reason to change from 204 to 200?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was having trouble updating the frontend component when removing a sku because 204 returned an empty response

@@ -1040,7 +1079,7 @@ def get(self):
user = get_authenticated_user()
account_number = marketplace_users.get_account_number(user)
if not account_number:
raise NotFound()
return []
Copy link
Member

Choose a reason for hiding this comment

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

Likewise, why go from 404 to 200? The client should be able to handle this.
An empty list should be returned in the case where the account number exists, but no subscriptions is attached to that user

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same reason as above, the billing ui would look like it was indefinitely loading when the api returned the 404. I found a way to properly handle the frontend though and updated the response codes accordingly

@Marcusk19 Marcusk19 requested a review from kleesc January 10, 2024 19:30
@Marcusk19 Marcusk19 merged commit 2a4ac09 into quay:master Jan 11, 2024
15 checks passed
Sunandadadi pushed a commit to Sunandadadi/quay that referenced this pull request Jan 15, 2024
* billing: marketplace UI

adds UI in billing section for managing user and org-bound skus

add more unit tests for org binding

changed endpoint for bulk attaching skus to orgs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants