-
Notifications
You must be signed in to change notification settings - Fork 1
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
PER-9866 [back-end] Endpoint for admins to update environment configu… #139
Conversation
d11149e
to
285d3f2
Compare
const validation = Joi.object() | ||
.keys({ | ||
...fieldsFromAdminAuthentication, | ||
description: Joi.string(), |
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.
description
should be required here too, per the specification
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.
Ok, it seemed a bit odd not to be required on creation but required on update. I thought it's a documentation mistake.
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.
Oh, you're actually right, description can't be required because this endpoint needs to be able to set description
to NULL by not sending it. Could you correct the error in the documentation so it matches the endpoint?
@@ -283,3 +287,182 @@ describe("POST /feature-flag", () => { | |||
expect(logger.error).toHaveBeenCalled(); | |||
}); | |||
}); | |||
|
|||
describe("PUT /feature-flag/1bdf2da6-026b-4e8e-9b57-a86b1817be5d", () => { |
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.
Nit: the URL param should be given as :featureFlagId
here, rather than using a specific value
description: 1, | ||
globallyEnabled: true, | ||
}) | ||
.expect(400); |
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.
Should the error code here be 404?
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 think the test is passing because it's not actually reaching the 404 error, it's getting a 400 because the ID in the URL params is not a UUID)
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 clicked the wrong button, meant to comment rather than approve)
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.
(Submitting a "Request changes" to reverse my previous accidental approval)
…ration New endpoint to update a feature-flag. There are validations on the globallyEnabled and description fields Unit and functional tests are covering all scenarios
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #139 +/- ##
==========================================
+ Coverage 91.08% 94.52% +3.43%
==========================================
Files 75 81 +6
Lines 1358 1497 +139
Branches 210 226 +16
==========================================
+ Hits 1237 1415 +178
+ Misses 110 75 -35
+ Partials 11 7 -4 ☔ View full report in Codecov by Sentry. |
…ration
New endpoint to update a feature-flag. There are validations on the globallyEnabled and description fields
Unit and functional tests are covering all scenarios