Closed lucasrodes closed 4 days ago
Quick links (staging server): Site | Admin | Wizard |
---|
Login: ssh owid@staging-site-chart-diff-conflict
Edited: 2024-06-26 13:00:29 UTC Execution time: 3.91 seconds
I ran into the following when testing:
..
to subtitle)Resolve conflict
STAGING
I see empty modal window
It's also a bit confusing that I'm given a choice for version
(do I need it?), title
(it's the same) and dimensions
(it's useful to see it, but I have no idea what to do with it) despite only touching subtitle
.
@lucasrodes please remove the ..
from the subtitle when you're done with this.
@Marigold I think this comes from the fact that I am using a st.experimental_dialog
(the conflict resolver modal) within a st.experimental_fragment
(the chart diff block).
According to their docs:
Dialogs can't contain fragments, and fragments can't contain dialogs.
😢
I think it works in indicator upgrader (we have dialogs for the 'explore mode') because the modal there only displays things (user can't interact with input widgets ー e.g. a button).
We can either get rid of the fragments
around chart diffs individual blocks or dialogs
around the conflict resolver.
I think I'll try to remove the st.experimental_dialog
and use a st.popover
for the conflict resolver instead.
The drawback of using popover
is that the HTML in it will be rendered even if the user does not click on it (similar to the issue with tabs). So, if we had several chart diffs with big conflicts, this could have a cost in performance.
However, I think that the performance increase from using fragment
is substantial and should outweigh the downsides from using popover
.
Also, it is rare that we have a page packed with several chart diffs with conflicts.
We detect conflicts currently based on whether a chart has an update timestamp more recent in master than in staging. This captures some conflicts but not all of them.
Solution
Create a new table:
chart_diff_conflicts
to track if a conflict was resolved. A conflict is uniquely identified bychart_id
andupdatedAt
(in target). If the conflict is resolved, we add an entry withconflict="resolved"
.Note: Should be merged after merging:
Context
In the diagram below, we show two scenarios:
Scenario 1: Reflects what we've been doing this far Scenario 2: Reflects a case when the user first branches out, and before updating a chart, someone goes and edits this chart on master.
Some definitions:
t0
: Timestamp of branching out (creation of staging server)ts
: Timestamp of chart edits on staging.ts1
,ts2
, etc. denote various chart edit timestamps on staging.tm
: Timestamp of chart edits on master/production