-
Notifications
You must be signed in to change notification settings - Fork 2.2k
feat: Add premium request usage report endpoints for organizations and users #3751
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
base: master
Are you sure you want to change the base?
Conversation
// UsageItem represents a single usage item in the enhanced billing platform report. | ||
type UsageItem struct { | ||
Date *string `json:"date"` | ||
Product *string `json:"product"` | ||
SKU *string `json:"sku"` | ||
Quantity *float64 `json:"quantity"` | ||
UnitType *string `json:"unitType"` | ||
PricePerUnit *float64 `json:"pricePerUnit"` | ||
GrossAmount *float64 `json:"grossAmount"` | ||
DiscountAmount *float64 `json:"discountAmount"` | ||
NetAmount *float64 `json:"netAmount"` | ||
RepositoryName *string `json:"repositoryName,omitempty"` | ||
// Organization name is only used for organization-level reports. | ||
OrganizationName *string `json:"organizationName,omitempty"` | ||
} | ||
|
||
// UsageReport represents the enhanced billing platform usage report response. | ||
type UsageReport struct { | ||
UsageItems []*UsageItem `json:"usageItems,omitempty"` | ||
} | ||
|
||
// PremiumRequestUsageItem represents a single usage line item in premium request usage reports. | ||
type PremiumRequestUsageItem struct { | ||
Product string `json:"product"` | ||
SKU string `json:"sku"` | ||
Model string `json:"model"` | ||
UnitType string `json:"unitType"` | ||
PricePerUnit float64 `json:"pricePerUnit"` | ||
GrossQuantity int `json:"grossQuantity"` | ||
GrossAmount float64 `json:"grossAmount"` | ||
DiscountQuantity int `json:"discountQuantity"` | ||
DiscountAmount float64 `json:"discountAmount"` | ||
NetQuantity int `json:"netQuantity"` | ||
NetAmount float64 `json:"netAmount"` | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used value types here for required fields. This follows a recent discussion at #3679 (comment), but doesn't match the existing type. I believe the old pattern happens everywhere, but it's better to refactor the one upon because they are too similar.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3751 +/- ##
==========================================
+ Coverage 91.11% 91.13% +0.01%
==========================================
Files 187 187
Lines 16702 16734 +32
==========================================
+ Hits 15218 15250 +32
Misses 1296 1296
Partials 188 188 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, @zyfy29!
One minor typo tweak, please, then we should be ready for a second LGTM+Approval from any other contributor to this repo before merging.
cc: @stevehipwell - @alexandear
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, @zyfy29!
LGTM.
// PremiumRequestUsageReportOptions specifies optional parameters | ||
// for the enhanced billing platform premium request usage report. | ||
type PremiumRequestUsageReportOptions struct { | ||
UsageReportOptions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Naming is indeed hard, and if you have recommendations for better names, please create "Suggested Changes" feedback in your code review and we can all discuss them if necessary. Thank you, @stevehipwell! |
@gmlewis my concern is that the naming in the current PR is correct in the context of the existing functions so I'm unsure if this PR is the correct place to suggest wholesale changes? If it is would we be pushing for the existing functions to be renamed too? |
Oh, I see. Yes, I think we should proceed with this PR and can you please open a new issue proposing widespread wholesale change recommendations, @stevehipwell ? |
Fixes: #3748.