Add first version of chart approval tool to admin UI #937
Conversation
Adds a set of small improvements to the suggested chart revision approval tool: - disables approve/reject button while new grapher is loading, in order to prevent accidental approval/rejection; - disables next/prev buttons while grapher is loading to address problem where incorrect grapher may be rendered if button is clicked multiple times too quickly; - adds timeout when rowNum input is changed, which address problem where incorrect grapher may be rendered if input is changed multiple times too quickly; - after a next/prev button click, updates rowNum using rowNumValid instead of rowNum itself to address problem where rowNum > totalNumRows.
Main changes: - Adds an `originalConfig` column to the `suggested_chart_revisions` SQL table, so that the original chart config prior to any revisions is persisted for historical reference. - Adds an `updatedBy` column to the `suggested_chart_revisions` SQL table. - Adds a unique key constraint to prevent accidental insert of duplicate suggested chart revisions. - Updates apiRouter with column changes.
Adds server-side validation to `status` update in POST `/suggested-chart-revisions/:suggestedChartRevisionId/update`.
…stedChartRevisionId`
Updates the chart approval UI to reflect recent database and API changes relating to suggested chart revisions.
There was a problem hiding this comment.
Nice work! I left some feedback that I think would be good to tackle before merging. Otherwise I think merging this soon is more important than perfecting various areas as we might want to do things a bit differently anyway once we improved things on the data infrastructure side and have more historic information about variables.
db/migration/1618193628982-CreateSuggestedChartRevisionsTable.ts
Outdated
Show resolved
Hide resolved
|
|
||
| @action.bound async rerenderGraphers() { | ||
| this._isGraphersSet = false | ||
| setTimeout(() => { |
There was a problem hiding this comment.
Why is this an imperative function that uses setTimeout? Could this not be solved with a computed property? I haven't followed the dependency chain to see if this would be difficult, just wondering about this as I read the code
There was a problem hiding this comment.
I initially tried getting this to work with a computed property but was having trouble getting the <Grapher /> component to re-render when the suggestedChartRevision state changed. Not sure what the component lifecycle problem is here.
This is similar to an issue in the chart editor where clicking on the "mobile" vs. "desktop" view does not induce a re-render.
|
Hi Bobbie! As we mentioned, this is impressive! I have made some changes that I pushed on a separate branch: feature/admin-suggested-chart-revision-approver...feature/admin-suggested-chart-revision-approver-tweaks The diff is bigger because I split up I will leave a bit more detailed comments tomorrow, but there isn't anything on my side that needs to be changed in order to merge and use this. |
Moves onClick state management changes into mobx actions for these two states: - showReadme - showSettings
Adds an inline comment clarifying the logic of when a grapher config will get overwritten. Also adds superfluous `canApprove` and `canReject` conditions prior to `saveGrapher` in order to make it more clear that there is some server- side protection above against unintentional config overwrites.
…lumn Adds an `isPendingOrFlagged` MySQL generated column to the `suggested_chart_revisions` table and adds this column to the unique key in place of `suggestedReason`.
Moves `db.transaction(...)` outside of `saveGrapher` so that `saveGrapher` can instead be passed a `TransactionContext` to use for executing the transaction logic. This allows `saveGrapher` to be used within other db transactions without having to use nested transactions.
… into feature/admin-suggested-chart-revision-approver
There was a problem hiding this comment.
@danyx23 @danielgavrilov This is all set to be merged, with one possible exception that you may want to review:
In commit 1dd0fc4, I refactor saveGrapher slightly so that it receives a db TransactionContext as a parameter instead of initializing a TransactionContext within the funcion itself. I've played around in the chart editor UI to check that its expected behavior has not changed, but it might be good for at least one of you to confirm that this change is acceptable.
This refactor solves a problem in the POST "/suggested-chart-revisions/:suggestedChartRevisionId/update" API method where saveGrapher was being called within a db transaction (thereby initializing a nested transaction). Passing a TransactionContext to saveGrapher as a parameter to avoid the nested transaction seemed like the easiest solution.
danielgavrilov
deleted the
feature/admin-suggested-chart-revision-approver
branch
Adds a UI to the admin site for approving/rejecting chart revisions that have been suggested by the
ChartRevisionSuggesterclass from theimportersrepository (added in this PR).Notion: Chart revision quality assurance tool v1.0
The purpose of this PR is to provide a layer of quality assurance for our charts that are updated by automated scripts (e.g. following a bulk dataset update). This PR creates a new MySQL table (
suggested_chart_revisions) for housing suggested chart revisions, alongside an admin UI for approving/rejecting the suggested revisions.A "suggested chart revision" is an amended OWID chart, but where the amendments have not yet been applied to the chart in question. If the suggested chart revision gets approved, then the amendments are applied to the chart (which overwrites and republishes the chart).
Deployed on
nightingalewith suggested chart revisions for World Bank World Development Indicator charts (to update data to current version). Try it out: