You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "geido (via GitHub)" <gi...@apache.org> on 2024/12/10 15:16:23 UTC
[PR] fix(Dashboard): Sync color configuration via dedicated endpoint [superset]
geido opened a new pull request, #31374:
URL: https://github.com/apache/superset/pull/31374
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(Dashboard): Sync color configuration via dedicated endpoint [superset]
Posted by "geido (via GitHub)" <gi...@apache.org>.
geido commented on code in PR #31374:
URL: https://github.com/apache/superset/pull/31374#discussion_r1882410842
##########
superset/commands/dashboard/update.py:
##########
@@ -202,3 +203,29 @@ def run(self) -> Model:
)
return configuration
+
+
+class UpdateDashboardColorsConfigCommand(UpdateDashboardCommand):
+ @transaction(
Review Comment:
You are right that this should be decorating the `run`. Just moved it. As for disabling the rule, that's to commit the modified date back to its original state when the `mark_updated` parameter is set to `False`. In all other cases, it should commit normally using the `@transaction`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(Dashboard): Sync color configuration via dedicated endpoint [superset]
Posted by "geido (via GitHub)" <gi...@apache.org>.
geido merged PR #31374:
URL: https://github.com/apache/superset/pull/31374
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(Dashboard): Sync color configuration via dedicated endpoint [superset]
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on code in PR #31374:
URL: https://github.com/apache/superset/pull/31374#discussion_r1882412466
##########
superset-frontend/src/dashboard/actions/dashboardState.js:
##########
@@ -867,23 +896,24 @@ export const ensureSyncedSharedLabelsColors =
const {
dashboardState: { sharedLabelsColorsMustSync },
} = getState();
- const updatedMetadata = { ...metadata };
const sharedLabelsColors = enforceSharedLabelsColorsArray(
metadata.shared_label_colors,
);
const freshLabelsColors = getFreshSharedLabels(
forceFresh ? [] : sharedLabelsColors,
);
const isSharedLabelsColorsSynced = isEqual(
- sharedLabelsColors,
- freshLabelsColors,
+ sortBy(sharedLabelsColors),
Review Comment:
why `sortBy` instead of just `sort`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(Dashboard): Sync color configuration via dedicated endpoint [superset]
Posted by "Antonio-RiveroMartnez (via GitHub)" <gi...@apache.org>.
Antonio-RiveroMartnez commented on code in PR #31374:
URL: https://github.com/apache/superset/pull/31374#discussion_r1882350771
##########
superset/commands/dashboard/update.py:
##########
@@ -202,3 +203,29 @@ def run(self) -> Model:
)
return configuration
+
+
+class UpdateDashboardColorsConfigCommand(UpdateDashboardCommand):
+ @transaction(
Review Comment:
Isn't this decorator supposed to be used on the `def run`? hence no need to disable the pylint rule about transaction
##########
superset/dashboards/api.py:
##########
@@ -767,6 +773,88 @@ def put_filters(self, pk: int) -> Response:
response = self.response_422(message=str(ex))
return response
+ @expose("/<pk>/colors", methods=("PUT",))
+ @protect()
+ @safe
+ @statsd_metrics
+ @event_logger.log_this_with_context(
+ action=lambda self, *args, **kwargs: f"{self.__class__.__name__}.put_colors",
+ log_to_statsd=False,
+ )
+ @requires_json
+ def put_colors(self, pk: int) -> Response:
+ """
+ Modify colors configuration for a dashboard.
+ ---
+ put:
+ summary: Update colors configuration for a dashboard.
+ parameters:
+ - in: path
+ schema:
+ type: integer
+ name: pk
+ - in: query
+ name: mark_updated
+ schema:
+ type: boolean
+ description: Whether to update the dashboard changed_on field
+ requestBody:
+ description: Colors configuration
+ required: true
+ content:
+ application/json:
+ schema:
+ $ref: '#/components/schemas/DashboardColorsConfigUpdateSchema'
+ responses:
+ 200:
+ description: Dashboard colors updated
+ content:
+ application/json:
+ schema:
+ type: object
+ properties:
+ result:
+ type: array
+ 400:
+ $ref: '#/components/responses/400'
+ 401:
+ $ref: '#/components/responses/401'
+ 403:
+ $ref: '#/components/responses/403'
+ 404:
+ $ref: '#/components/responses/404'
+ 422:
+ $ref: '#/components/responses/422'
+ 500:
+ $ref: '#/components/responses/500'
+ """
+ try:
+ item = self.update_colors_model_schema.load(request.json, partial=True)
Review Comment:
Since we can send just partial updates, isn't better to use a PATCH call instead?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(Dashboard): Sync color configuration via dedicated endpoint [superset]
Posted by "geido (via GitHub)" <gi...@apache.org>.
geido commented on code in PR #31374:
URL: https://github.com/apache/superset/pull/31374#discussion_r1882406097
##########
superset/dashboards/api.py:
##########
@@ -767,6 +773,88 @@ def put_filters(self, pk: int) -> Response:
response = self.response_422(message=str(ex))
return response
+ @expose("/<pk>/colors", methods=("PUT",))
+ @protect()
+ @safe
+ @statsd_metrics
+ @event_logger.log_this_with_context(
+ action=lambda self, *args, **kwargs: f"{self.__class__.__name__}.put_colors",
+ log_to_statsd=False,
+ )
+ @requires_json
+ def put_colors(self, pk: int) -> Response:
+ """
+ Modify colors configuration for a dashboard.
+ ---
+ put:
+ summary: Update colors configuration for a dashboard.
+ parameters:
+ - in: path
+ schema:
+ type: integer
+ name: pk
+ - in: query
+ name: mark_updated
+ schema:
+ type: boolean
+ description: Whether to update the dashboard changed_on field
+ requestBody:
+ description: Colors configuration
+ required: true
+ content:
+ application/json:
+ schema:
+ $ref: '#/components/schemas/DashboardColorsConfigUpdateSchema'
+ responses:
+ 200:
+ description: Dashboard colors updated
+ content:
+ application/json:
+ schema:
+ type: object
+ properties:
+ result:
+ type: array
+ 400:
+ $ref: '#/components/responses/400'
+ 401:
+ $ref: '#/components/responses/401'
+ 403:
+ $ref: '#/components/responses/403'
+ 404:
+ $ref: '#/components/responses/404'
+ 422:
+ $ref: '#/components/responses/422'
+ 500:
+ $ref: '#/components/responses/500'
+ """
+ try:
+ item = self.update_colors_model_schema.load(request.json, partial=True)
Review Comment:
PATCH isn't widely supported in Superset at the moment. I could not find a single endpoint using PATCH atm
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(Dashboard): Sync color configuration via dedicated endpoint [superset]
Posted by "kgabryje (via GitHub)" <gi...@apache.org>.
kgabryje commented on code in PR #31374:
URL: https://github.com/apache/superset/pull/31374#discussion_r1882426321
##########
superset-frontend/src/utils/colorScheme.ts:
##########
@@ -91,21 +93,40 @@ export const getSharedLabelsColorMapEntries = (
* @param customLabelsColor - the custom label colors in label_colors field
* @returns all color entries except custom label colors
*/
-export const getLabelsColorMapEntries = (
+export const getFreshLabelsColorMapEntries = (
customLabelsColor: Record<string, string> = {},
): Record<string, string> => {
const labelsColorMapInstance = getLabelsColorMap();
const allEntries = Object.fromEntries(labelsColorMapInstance.getColorMap());
// custom label colors are applied and stored separetely via label_colors
- // removing all instances of custom label colors from the entries
Object.keys(customLabelsColor).forEach(label => {
delete allEntries[label];
});
return allEntries;
};
+/**
+ * Returns all dynamic labels and colors (excluding custom label colors).
+ *
+ * @param labelsColorMap - the labels color map
+ * @param customLabelsColor - the custom label colors in label_colors field
+ * @returns all color entries except custom label colors
+ */
+export const getDynamicLabelsColors = (
+ fullLabelsColors: Record<string, string>,
+ customLabelsColor: Record<string, string> = {},
+): Record<string, string> => {
+ const labelsColors = { ...fullLabelsColors };
Review Comment:
```suggestion
return omit(fullLabelsColors, Object.keys(customLabelsColor))
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org