Per 10160 add storage adjustment endpoint #588
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #588 +/- ##
==========================================
- Coverage 98.25% 98.23% -0.02%
==========================================
Files 102 102
Lines 2458 2491 +33
Branches 410 412 +2
==========================================
+ Hits 2415 2447 +32
- Misses 39 40 +1
Partials 4 4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
slifty
left a comment
slifty
left a comment
There was a problem hiding this comment.
I have some thoughts for consideration about shifting some of the types and routes to lean into REST (even if we aren't literally storing the results of these endpoints as entities in the database, and even if we aren't going to implement anything but the POST).
As always: if you don't agree no worries at all and I can give another review with that consensus in mind!
| apiRoutes.use("/archive", archiveController); | ||
| apiRoutes.use("/admin", adminController); | ||
| apiRoutes.use("/billing", billingController); | ||
| apiRoutes.use("/billing", storageController); // Deprecated path maintained to avoid breaking changes |
There was a problem hiding this comment.
w00t!
How do we want to document deprecation timelines? I imagine we will eventually want to delete this line but if we don't track that plan externally (either with a deprecation calendar document and / or an issue that notes our intention) it feels like it might end up being a forever thing.
| ); | ||
|
|
||
| storageController.post( | ||
| "/adjustment", |
There was a problem hiding this comment.
API design question: Might it make sense to think of StorageAdjustment as an entity? Then this could be a REST endpoint and I'd advocate for pluralizing this route so we create via POST /storage/adjustments. We would then basically translate the internal ledger entry to a slimmed-down version StorageAdjustment.
And then some day we could get collections of adjustments via GET /storage/adjustments, but maybe that way will never come and that's still OK since I think the noun is still more consistent with our RESTful intentions.
I realize under the hood we're using ledger entries (and that entity will pretty much always be an internal concept / would probably need a meaningful refactor before exposing it more directly).
| export const storageController = Router(); | ||
|
|
||
| storageController.post( | ||
| "/gift", |
There was a problem hiding this comment.
Similar to my adjustment question: if we think of this as StorageGift entity, then this would be a plural route, i.e. "create a gift by invoking POST /storage/gifts"
| verifyAdminAuthentication, | ||
| async (req: Request, res: Response, next: NextFunction) => { | ||
| try { | ||
| validateAdjustStorageRequest(req.body); |
There was a problem hiding this comment.
It might be more RESTful if we think of the targets of these HTTP actions as nouns rather than verbs.
So validateStorageAdjustmentRequest rather than validateAdjustStorageRequest -- "I want to create a storage adjustment" rather than "I want to create an adjust storage"
| async (req: Request, res: Response, next: NextFunction) => { | ||
| try { | ||
| validateAdjustStorageRequest(req.body); | ||
| const result = await adjustStorage(req.body); |
There was a problem hiding this comment.
If we use the body-is-an-entity approach then this might be renamed invokeStorageAdjustment or applyStorageAdjustment
packages/api/src/storage/service.ts
Outdated
| import { db } from "../database"; | ||
| import type { GiftStorageRequest, GiftStorageResponse } from "./models"; | ||
| import type { | ||
| AdjustStorageRequest, |
There was a problem hiding this comment.
This is a broader thought about the overall Req / Res pattern in Stela -- and so this comment is not to actually change the pattern, but to just raise the idea for a future issue.
We might consider thinking of defining REST entities without the Request and Response suffix.
For instance, here we could simply have StorageAdjustment and StorageGift entities, which each might have some readonly properties. The request form would be a modified version of that entity with readonly properties stripped (OTS has a TS utility class that accomplishes this, I could nudge them to publish it to npm for us to use!)
This approach helps enforce adherence to REST principals via DRY TypeScript:
- Define a single
Foointerface which has some attributes asreadonly - POST request body:
Writable<Foo> - PATCH request body:
Partial<Writable<Foo>> - GET / POST / PATCH response body:
Foo
I don't wanna distract at all from this PR though, (and sorry for opening this line here when it's by no means in scope). I should make an issue to explore the idea / that's probably the right place to actually discuss if it's an approach you'd be interested in.
liam-lloyd
force-pushed
the
per-10160_add_storage_adjustment_endpoint
branch
4 times, most recently
from
c881669 to
3ec4a7b
Compare
liam-lloyd
force-pushed
the
per-10160_add_storage_adjustment_endpoint
branch
5 times, most recently
from
5ebe036 to
e720fb6
Compare
Storage is a more accurate name for this resource; this repo is unlikely to directly handle billing since that is handled by Stripe, but it does work with storage. The old /billing path is maintained for backward compatibility.
Occassionally we encounter a situation where we would like Permanent staff to be able to update the storage in an account. This commit adds an admin endpoint to make such updates.
liam-lloyd
force-pushed
the
per-10160_add_storage_adjustment_endpoint
branch
from
e720fb6 to
0cb8807
Compare
No description provided.