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.

Sign up for GitHub

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

Jump to bottom

🎉 (admin) add indicator chart editor #3886

Closed
wants to merge 2 commits into from
Closed

🎉 (admin) add indicator chart editor #3886

wants to merge 2 commits into from

Conversation

sophiamersmann
Copy link
Member

@sophiamersmann sophiamersmann commented Aug 20, 2024 •
edited
Loading

Adds an editor for indicator-level chart configs.

Details

  • The indicator chart editor
    • A small note in the Basic tab states that this is an indicator chart
    • The Inheritance tab lists all charts that currently inherit from the indicator or where inheritance might be enabled
    • The dimensions field can't be edited (since it can't be inherited)
    • The Debug tab lists all configs (the admin config, etl config and the merged config)
  • The variable page (under /variables/123456) has a new "Partial Grapher config" section that shows the ETL and the admin version. It comes with edit/delete buttons for the admin-authored config.

We should probably also add a few more tests for the new grapherConfigAdmin APIs, but I ran out of time :(

Knitted with a hot needle!

@sophiamersmann sophiamersmann mentioned this pull request Aug 20, 2024
Merged
3 tasks
@sophiamersmann Graphite App
Copy link
Member Author

sophiamersmann commented Aug 20, 2024 •
edited by danyx23
Loading

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @sophiamersmann and the rest of your teammates on Graphite Graphite

@owidbot
Copy link
Contributor

owidbot commented Aug 20, 2024 •
edited
Loading

Quick links (staging server):

Site Admin Wizard

Login: ssh owid@staging-site-indicator-chart-editor

SVG tester:

Number of differences (default views): 0 ✅
Number of differences (all views): 0 ✅

Edited: 2024-08-21 11:53:23 UTC
Execution time: 1.21 seconds

@sophiamersmann sophiamersmann force-pushed the inherit-indicator-settings branch from 89f1b6f to 3905b10 Compare August 21, 2024 09:39
@sophiamersmann sophiamersmann force-pushed the indicator-chart-editor branch from e9aa069 to ddded8d Compare August 21, 2024 09:39
@sophiamersmann sophiamersmann force-pushed the inherit-indicator-settings branch from 3905b10 to caeefb7 Compare August 21, 2024 11:39
@sophiamersmann sophiamersmann force-pushed the indicator-chart-editor branch 5 times, most recently from c361c9d to 29067f4 Compare August 21, 2024 13:24
@sophiamersmann sophiamersmann marked this pull request as ready for review August 21, 2024 13:41
@sophiamersmann sophiamersmann requested a review from danyx23 August 21, 2024 13:41
@danyx23 danyx23 force-pushed the inherit-indicator-settings branch from e4f5231 to 7fd998a Compare August 23, 2024 11:50
@danyx23 danyx23 force-pushed the indicator-chart-editor branch from 29067f4 to 582d094 Compare August 23, 2024 11:50
sophiamersmann
sophiamersmann commented Aug 26, 2024
View reviewed changes
adminSiteClient/VariableEditPage.tsx
undefined,
"DELETE"
)
window.location.reload()
Copy link
Member Author

Choose a reason for hiding this comment

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

While talking about this in our dev meeting, I suddenly realized that I had left this criminal line in here! I'll replace it with an optimistic update in the next few days... :)

Copy link
Contributor

Choose a reason for hiding this comment

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

this seems to still be here :)

This was referenced Aug 26, 2024
Merged
Merged
Merged
Merged
Closed
Closed
Closed
Closed
Merged
Closed
Merged
Merged
Draft
@danyx23 danyx23 mentioned this pull request Aug 26, 2024
Merged
danyx23
danyx23 requested changes Sep 2, 2024
View reviewed changes
Copy link
Contributor

@danyx23 danyx23 left a comment •
edited by sophiamersmann
Loading

Choose a reason for hiding this comment

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

Comment moved to #3793

@danyx23
Copy link
Contributor

danyx23 commented Sep 2, 2024

Hey @sophiamersmann, the Github Pull Requests Vs code extension mis-attributed the above review to this PR instead of the one below. I hope this doesn't cause any issues on your side - the things going wrong that I described apply to the PR one level down already and should be fixed there. I'll now review this PR.

@sophiamersmann
Copy link
Member Author

I'll move your comment to the other PR then, so that it doesn't get too confusing.

@sophiamersmann sophiamersmann force-pushed the inherit-indicator-settings branch from 7fd998a to cffbb09 Compare September 2, 2024 16:01
danyx23
danyx23 reviewed Sep 2, 2024
View reviewed changes
Copy link
Contributor

@danyx23 danyx23 left a comment

Choose a reason for hiding this comment

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

Looks good! The FieldsRow on the variables page that shows the two partial configs next to each other could benefit from an aling-items: top instead of center imho as the two boxes seem unbalanced in the example I looked at.

I'm a little concerned about the perf of the queries to get all charts for an indciator since it joins on a view - but we can go ahead like this and monitor the perf and change it in a subequent pr if it is a problem.

db/model/Variable.ts
FROM charts c
JOIN chart_configs cc ON cc.id = c.configId
JOIN chart_dimensions cd ON cd.chartId = c.id
LEFT JOIN charts_x_parents cxp ON c.id = cxp.chartId
Copy link
Contributor

Choose a reason for hiding this comment

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

This join is probably fine but ideally we'd have an eye on performance. Since this joins on a view I'm not sure if it can make use of an index - I think it has to do a full table scan.

Maybe charts_x_parents should be a proper table maintained with triggers instead. I've asked Lars and Mojmir if we have any kind or perf metrics about slow queries on prod mysql. We probably don't have to tackle this as part of this PR though.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point! I just checked where this query is used, and at the moment, this particular query is only used once in the indicator chart admin to compile a list of charts that inherit from that indicator. So, it's not a very critical path. Let's move forward then and come back to it when we need to :)

adminSiteClient/VariableEditPage.tsx
undefined,
"DELETE"
)
window.location.reload()
Copy link
Contributor

Choose a reason for hiding this comment

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

this seems to still be here :)

@danyx23 danyx23 force-pushed the inherit-indicator-settings branch from cffbb09 to e6093e9 Compare September 3, 2024 07:59
@danyx23 danyx23 force-pushed the indicator-chart-editor branch from 9fb6caf to ec770cf Compare September 3, 2024 07:59
@sophiamersmann sophiamersmann force-pushed the inherit-indicator-settings branch from e6093e9 to 288f511 Compare September 3, 2024 09:51
@sophiamersmann sophiamersmann force-pushed the inherit-indicator-settings branch 2 times, most recently from eff6e94 to 949f3f1 Compare September 3, 2024 13:03
@danyx23
Copy link
Contributor

danyx23 commented Sep 3, 2024

Looks great! I found one little thing still:

  • When you go to an existing single indicator chart and enable inheritance, then in the debug tab go to edit parent chart, then make some changes and save then the button text for "create indicator chart" doens't change, leaving you in the dark about whether your changes were saved

@danyx23 danyx23 force-pushed the indicator-chart-editor branch from 4aba1f2 to 73d29e8 Compare September 3, 2024 14:18
@sophiamersmann
Copy link
Member Author

Changes are folded into its parent, #3793

@danyx23 danyx23 mentioned this pull request Sep 5, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danyx23 danyx23 danyx23 approved these changes

Assignees

@sophiamersmann sophiamersmann

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sophiamersmann @owidbot @danyx23