Skip to content
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

add warning_three_column_groupby #3115

Closed
wants to merge 3 commits into from

Conversation

alison985
Copy link
Contributor

See mozilla#555 - this is a port upstream of a Mozilla feature.

This PR will

  • display a warning and not let a visualization be saved if there are 3 columns in the resultset of the query and there is not a group by field chosen in the visualization configuration.
  • make the warning disappear and enable saving once a group by field is choosen.

Error state:
screenshot 2018-11-25 15 35 02

Post-error state:
screenshot 2018-11-25 15 36 25

@alison985
Copy link
Contributor Author

@jezdez I can't add you as a reviewer, but a heads up that this is ready to review.

@arikfr
Copy link
Member

arikfr commented Nov 26, 2018

Here's a valid chart this doesn't let me save:

image

(https://deploy-preview-3115--redash-preview.netlify.com/queries/22/source#32)

The data is: month, # of issues, # of pull requests. There is no problem of plotting each one of them on its own or both of them together.

The issue you seem to try and overcome is when there are multiple Y values for a single X value (#1742). But this needs to be checked after the data is arranged into series. Also following the discussion in #1742 and in the forum, I inclining towards conceding to applying an aggregation in such cases...

@@ -32,6 +38,24 @@ const EditVisualizationDialog = {
this.visualization = this.newVisualization();
}

this.has3plusColumnsFunction = () => {
let has3plusColumns = false;
if ((JSON.stringify(this.visualization.options.columnMapping).match(/,/g) || []).length > 1) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this and not _.size?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great point - a way easier read. My brain just got stuck on it being a string.


this.disableSubmit = () => {
if (this.has3plusColumnsFunction() &&
JSON.stringify(this.visualization.options.columnMapping).includes('unused') &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a query where one of the columns named "unused"... so this is not so reliable.

But this can be tested with values(this.visualization.options.columnMapping).includes('unused').

(values is imported from lodash)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@alison985
Copy link
Contributor Author

@arikfr I have to say I personally agree with the "applying an aggregation in such cases" route. It's a more elegant solution for the user as long as they get information about why they can't disable the aggregation if they attempt to disable it. Therefore I am not going to pursue the bug you found about multiple measures.

@jezdez FYI.

@arikfr
Copy link
Member

arikfr commented Jan 3, 2019

As this introduces more issues than solving anything, I'm closing this. I'm happy to discuss a potential solution along the lines of what I mentioned in the end of my previous comment.

Closing this now.

@arikfr arikfr closed this Jan 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants