feat: ability to upsert single legal values #9056
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Files |
There was a problem hiding this comment.
Regarding your decisions, I don't agree with this one:
since it allows to add a new legal value or update description of a single legal value I decided to go for the PUT method since we know the identity of the legal value (context name + legal value name combination is unique).
Because we're putting to :contextField/legalValues (why isn't it :contextField/legal-values?), I would expect the PUT to replace all the legal values (in which case, it should've also been a list). So in this case, because you're only sending a single value and want that added to the collection, I think POST is the right method to use.
(That way, it'd even make sense to allow sending either a single value or a list of values later, if that's something people want)
On the other hand, if you sent it to :contextField/legal-values/:legalValueName, then I think PUT would be the right verb.
As for reusing the context field updated event: yeah, that sounds right to me.
| const validatedLegalValue = await legalValueSchema.validateAsync( | ||
| contextFieldLegalValue.legalValue, | ||
| ); |
There was a problem hiding this comment.
Maybe I'm missing something, but why are we using the schema to validate here? Isn't that automatically taken care of at the endpoint level?
There was a problem hiding this comment.
Oh, also, legalValueSchema is not LegalValueSchema from ../../openapi. I thought it looked like Joi, but was a little confused by the import (and didn't look hard enough, apparently).
But everything that the joi schema does, we should be able to do with openapi directly, right? Set string min and max lengths and allow description to be null, undefined, or an arbitrary string?
There was a problem hiding this comment.
It's consistent with our current approach in the context module where openapi validates simple string and joi actual 1-100 characters. I didn't want to introduce a new convention in this PR
There was a problem hiding this comment.
Yeah, that's fair. I suspect we'll just forget about it and never touch it, then. But that's fine too. It's a little added complexity, but not a big deal here.
| const existingIndex = legalValues.findIndex( | ||
| (legalvalue) => legalvalue.value === validatedLegalValue.value, | ||
| ); |
There was a problem hiding this comment.
Oh, my. we store this in a list, huh? I imagine using a map would be better, but it's probably also not something we want to touch now, I guess 😅
There was a problem hiding this comment.
But then again, maybe order is important? 🤷🏼
There was a problem hiding this comment.
Yes, this PR piggybacks on the current implementation
About the changes
Ability to add/update a single legal value for the context.
Important decisions:
Addresses: #8934
Followup PRs:
Important files
Discussion points