-
Notifications
You must be signed in to change notification settings - Fork 6
UID2-2885 Optout UI Improvement #600
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: main
Are you sure you want to change the base?
Conversation
| private void handlePartnerConfigGet(RoutingContext rc) { | ||
| try { | ||
| final String partnerName = rc.pathParam("partner_name"); | ||
| if (partnerName == null) { |
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.
Maybe also check if this is an empty 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.
Added check for empty string
| } | ||
|
|
||
| String config = this.partnerConfigProvider.getConfig(); | ||
| JsonArray allPartnerConfigs = new JsonArray(config); |
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.
Can we put these into one line like you do for the other methods
| JsonArray allPartnerConfigs = new JsonArray(config); | ||
|
|
||
| // Look for the specific partner | ||
| for (int i = 0; i < allPartnerConfigs.size(); i++) { |
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.
You reuse a similar logic in most of these methods, can we look at putting this into a reusable function
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.
Added findPartnerIndex helper fn
|
|
||
| private void handlePartnerConfigAdd(RoutingContext rc) { | ||
| try { | ||
|
|
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: Remove this space
|
|
||
| rc.response() | ||
| .putHeader(HttpHeaders.CONTENT_TYPE, "application/json") | ||
| .end("\"success\""); |
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.
Can we respond with the new partner config that we added
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.
Changed to respond with new config
|
|
||
| rc.response() | ||
| .putHeader(HttpHeaders.CONTENT_TYPE, "application/json") | ||
| .end("\"success\""); |
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.
Can we respond with the new updated object
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.
Changed to respond with updated object
| ResponseUtil.error(rc, 400, "Partner name is required"); | ||
| return; | ||
| } | ||
| final String partnerName = partnerNames.getFirst(); |
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.
Why do we accept a list of partnerNames but only remove the first partner?
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 RoutingContext.queryParam is a list of Strings since you can always specify multiple values for each query param eg. /delete/?partner_name=partner1&partner_name=partner2 despite only expecting one partner_name here. src/main/java/com/uid2/admin/vertx/service/SiteService.java has the same behaviour in handleSiteAdd
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.
Got it, thanks
| private void handlePartnerConfigBulkReplace(RoutingContext rc) { | ||
| try { | ||
| // refresh manually | ||
| this.partnerConfigProvider.loadContent(); |
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.
Can we refresh the provider manually for all methods before retrieving the config?
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.
Added manual refresh in all methods
| try { | ||
|
|
||
| JsonObject newConfig = rc.body().asJsonObject(); | ||
| if (newConfig == null) { |
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 we put this check in the validatePartnerConfig method as well?
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.
Yep added null check to validatePartnerConfig. I've left the existing checks in so we can give better errors
| storageManager.upload(allPartnerConfigs); | ||
| rc.response() | ||
| .putHeader(HttpHeaders.CONTENT_TYPE, "application/json") | ||
| .end("\"success\""); |
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.
Can we also return the deleted object here? We can return a new object similar to what we do for ClientSideKeyPairs like:
{
success: true,
deleted_partner: allPartnerConfigs.get(existingPartnerIdx)
}
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.
Updated to return deleted object. Do you think it's sufficient to return 200 + deleted object or would you still prefer to add success: true as well?
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.
Yes that's fine, returning the deleted object was the main thing I wanted!
|
|
||
| storageManager.upload(partners); | ||
|
|
||
| rc.response() |
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.
Can we also return the replaced partner config here
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.
Now returns replace config
| } | ||
| }, new AuditParams(Collections.emptyList(), List.of("partner_id", "config")), Role.PRIVILEGED)); | ||
| }, new AuditParams(Collections.emptyList(), List.of("name")), Role.MAINTAINER)); | ||
| router.delete(API_PARTNER_CONFIG_DELETE.toString()).blockingHandler(auth.handle((ctx) -> { |
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.
Lets also put the DELETE operation in the danger zone as a PRIVILEGED role
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.
Moved to danger zone and updated to Role.PRIVILEGED
| this.handlePartnerConfigDelete(ctx); | ||
| } | ||
| }, new AuditParams(List.of("partner_name"), Collections.emptyList()), Role.MAINTAINER)); | ||
| router.post(API_PARTNER_CONFIG_BULK_REPLACE.toString()).blockingHandler(auth.handle((ctx) -> { |
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.
Let's make bulk replace for Role.SUPERUSER
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.
Updated to Role.SUPERUSER
API Changes:
GET /api/partner_config/list- List all partner configsGET /api/partner_config/get/{partner_name}- Get specific partner configPOST /api/partner_config/add- Add new partnerPUT /api/partner_config/update- Update existing partnerDELETE /api/partner_config/delete- Delete partnerPOST /api/partner_config/bulk_replace- Replace all configs (renamed fromupdateand moved to Danger Zone)UI Changes:
OptOut Partner Managementpage to reflect API changes