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

feat: describe altered nodes and prompt for summary message before publishing #683

Merged
merged 20 commits into from
Dec 8, 2021
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
ef84b40
log altered nodes
jessicamcinchak Oct 20, 2021
779ff3c
clear msg to say unpublished flow
jessicamcinchak Oct 20, 2021
e2257c3
Merge branch 'main' of github.com:theopensystemslab/planx-new into je…
jessicamcinchak Oct 27, 2021
1c7bed7
first draft /flows/:flowId/diff endpoint
jessicamcinchak Oct 27, 2021
1d039d0
Merge branch 'main' of github.com:theopensystemslab/planx-new into je…
jessicamcinchak Oct 28, 2021
5165b3f
mock publish dialog box
jessicamcinchak Oct 28, 2021
c60dc0b
show summary of changes
jessicamcinchak Oct 28, 2021
9ac1acd
explain changes in dialog
jessicamcinchak Oct 29, 2021
7ba1731
add nullable summary text col to published_flows table
jessicamcinchak Nov 2, 2021
7b158f0
publish change summary isn't required, db should save null not undefined
jessicamcinchak Nov 2, 2021
8bc097e
fix change message for initial publish
jessicamcinchak Nov 2, 2021
6c6006b
Merge branch 'main' of github.com:theopensystemslab/planx-new into je…
jessicamcinchak Nov 3, 2021
91bcb18
Merge branch 'main' of github.com:theopensystemslab/planx-new into je…
jessicamcinchak Nov 16, 2021
3beb9d3
pr feedback & cleanup
jessicamcinchak Nov 16, 2021
0d341e8
Merge branch 'main' of github.com:theopensystemslab/planx-new into je…
jessicamcinchak Dec 7, 2021
5500099
remove unused import
jessicamcinchak Dec 7, 2021
d505c75
Merge branch 'main' of github.com:theopensystemslab/planx-new into je…
jessicamcinchak Dec 8, 2021
e435886
axios.post, next api error handling, hasura index, wrap caller in try…
jessicamcinchak Dec 8, 2021
a1c87ac
one more try/catch block
jessicamcinchak Dec 8, 2021
1127565
commit down.sql too
jessicamcinchak Dec 8, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 38 additions & 5 deletions api.planx.uk/publish.js
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,37 @@ const dataMerged = async (id, ob = {}) => {
return ob;
};

const publishFlow = async (req, res) => {
const diffFlow = async (req, res, next) => {
if (!req.user?.sub)
return res.status(401).json({ error: "User ID missing from JWT" });
Copy link
Contributor

@johnrees johnrees Dec 8, 2021

Choose a reason for hiding this comment

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

nit, could this be changed to use the next() callback please

next({
status: 401,
message: "user failed to authenticate.",
});

I'm still not exactly completely happy with the api error handling but if everything goes to the same function in the chain it'll be easier to refactor in future

Copy link
Member Author

Choose a reason for hiding this comment

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

yep thanks for spotting these lingering cases - all updated to use next() now!


try {
const flattenedFlow = await dataMerged(req.params.flowId);
const mostRecent = await getMostRecentPublishedFlow(req.params.flowId);

const delta = jsondiffpatch.diff(mostRecent, flattenedFlow);

if (delta) {
const alteredNodes = Object.keys(delta).map((key) => ({
id: key,
...flattenedFlow[key]
}));

res.json({
alteredNodes
});
} else {
res.json({
alteredNodes: null,
message: "No new changes",
});
}
} catch (error) {
next(error);
}
};

const publishFlow = async (req, res, next) => {
if (!req.user?.sub)
return res.status(401).json({ error: "User ID missing from JWT" });

Expand All @@ -84,12 +114,14 @@ const publishFlow = async (req, res) => {
mutation PublishFlow(
$data: jsonb = {},
$flow_id: uuid,
$publisher_id: Int,
$publisher_id: Int,
$summary: String,
) {
insert_published_flows_one(object: {
data: $data,
flow_id: $flow_id,
publisher_id: $publisher_id,
summary: $summary,
}) {
id
flow_id
Expand All @@ -103,8 +135,10 @@ const publishFlow = async (req, res) => {
data: flattenedFlow,
flow_id: req.params.flowId,
publisher_id: parseInt(req.user.sub, 10),
summary: req.query?.summary || null,
}
);

const publishedFlow =
response.insert_published_flows_one &&
response.insert_published_flows_one.data;
Expand All @@ -124,9 +158,8 @@ const publishFlow = async (req, res) => {
});
}
} catch (error) {
console.error(error);
res.status(500).json({ error });
next(error);
}
};

module.exports = { publishFlow };
module.exports = { diffFlow, publishFlow };
4 changes: 3 additions & 1 deletion api.planx.uk/server.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ const {

const { signS3Upload } = require("./s3");
const { locationSearch } = require("./gis/index");
const { publishFlow } = require("./publish");
const { diffFlow, publishFlow } = require("./publish");

// debug, info, warn, error, silent
const LOG_LEVEL = process.env.NODE_ENV === "test" ? "silent" : "debug";
Expand Down Expand Up @@ -444,6 +444,8 @@ app.get("/throw-error", () => {
throw new Error("custom error");
});

app.post("/flows/:flowId/diff", useJWT, diffFlow);

app.post("/flows/:flowId/publish", useJWT, publishFlow);

// unauthenticated because accessing flow schema only, no user data
Expand Down
114 changes: 107 additions & 7 deletions editor.planx.uk/src/pages/FlowEditor/components/PreviewBrowser.tsx
Original file line number Diff line number Diff line change
@@ -1,13 +1,19 @@
import Box from "@material-ui/core/Box";
import Button from "@material-ui/core/Button";
import Dialog from "@material-ui/core/Dialog";
import DialogActions from "@material-ui/core/DialogActions";
import DialogContent from "@material-ui/core/DialogContent";
import DialogTitle from "@material-ui/core/DialogTitle";
import { makeStyles } from "@material-ui/core/styles";
import Tooltip from "@material-ui/core/Tooltip";
import Typography from "@material-ui/core/Typography";
import formatDistanceToNow from "date-fns/formatDistanceToNow";
import React, { useEffect, useState } from "react";
import { ExternalLink, Globe, RefreshCw, Terminal } from "react-feather";
import { useAsync } from "react-use";
import Input from "ui/Input";

import { TYPES } from "../../../@planx/components/types";
import Questions from "../../Preview/Questions";
import { useStore } from "../lib/store";

Expand Down Expand Up @@ -66,6 +72,31 @@ const DebugConsole = () => {
);
};

function PublishChangeItem(props: any) {
const { node } = props;
let text, data;

if (node.id === "_root") {
text = "Changed _root service by adding, deleting or re-ordering nodes";
} else if (node.id === "0") {
text = `The entire _root service will be published for the first time`;
} else if (node.id && Object.keys(node).length === 1) {
text = `Deleted node ${node.id}`;
} else if (node.type && node.data) {
text = `Added/edited ${TYPES[node.type]}`;
data = JSON.stringify(node.data, null, 2);
} else {
text = `Added/edited ${TYPES[node.type]}`;
}

return (
<>
<Typography variant="body2">{text}</Typography>
{data && <pre style={{ fontSize: ".8em" }}>{data}</pre>}
</>
);
}

const PreviewBrowser: React.FC<{ url: string }> = React.memo((props) => {
const [showDebugConsole, setDebugConsoleVisibility] = useState(false);
const [
Expand All @@ -75,16 +106,23 @@ const PreviewBrowser: React.FC<{ url: string }> = React.memo((props) => {
publishFlow,
lastPublished,
lastPublisher,
diffFlow,
] = useStore((state) => [
state.id,
state.resetPreview,
state.setPreviewEnvironment,
state.publishFlow,
state.lastPublished,
state.lastPublisher,
state.diffFlow,
]);
const [key, setKey] = useState<boolean>(false);
const [lastPublishedTitle, setLastPublishedTitle] = useState<string>();
const [lastPublishedTitle, setLastPublishedTitle] = useState<string>(
"This flow is not published yet"
);
const [alteredNodes, setAlteredNodes] = useState<object[]>();
const [dialogOpen, setDialogOpen] = useState<boolean>(false);
const [summary, setSummary] = useState<string>();
const classes = useStyles();

useEffect(() => setPreviewEnvironment("editor"), []);
Expand Down Expand Up @@ -152,18 +190,80 @@ const PreviewBrowser: React.FC<{ url: string }> = React.memo((props) => {
variant="contained"
color="primary"
onClick={async () => {
setLastPublishedTitle("Sending changes...");
const publishedFlow = await publishFlow(flowId);
setLastPublishedTitle("Checking for changes...");
const alteredFlow = await diffFlow(flowId);
setAlteredNodes(
alteredFlow?.data.alteredNodes
? alteredFlow.data.alteredNodes
: []
);
setLastPublishedTitle(
publishedFlow?.data.alteredNodes
? `Successfully published changes to ${publishedFlow.data.alteredNodes.length} node(s)`
alteredFlow?.data.alteredNodes
? `Found changes to ${alteredFlow.data.alteredNodes.length} node(s)`
: "No new changes to publish"
);
setDialogOpen(true);
}}
disabled={window.location.hostname.endsWith("planx.uk")}
>
PUBLISH
CHECK FOR CHANGES TO PUBLISH
</Button>
<Dialog
open={dialogOpen}
onClose={() => setDialogOpen(false)}
aria-labelledby="alert-dialog-title"
aria-describedby="alert-dialog-description"
>
<DialogTitle style={{ paddingBottom: 0 }}>
{lastPublishedTitle}
</DialogTitle>
<DialogContent>
{alteredNodes?.length ? (
<>
<Box pb={1}>
<ul>
{alteredNodes.map((a: any) => (
<li key={a.id}>
<PublishChangeItem node={a} />
</li>
))}
</ul>
</Box>
<Input
bordered
type="text"
name="summary"
value={summary || ""}
placeholder="Summarise your changes..."
onChange={(e) => setSummary(e.target.value)}
/>
</>
) : (
`No new changes to publish`
)}
</DialogContent>
<DialogActions>
<Button onClick={() => setDialogOpen(false)}>
KEEP EDITING
</Button>
<Button
color="primary"
autoFocus
onClick={async () => {
setDialogOpen(false);
setLastPublishedTitle("Publishing changes...");
const publishedFlow = await publishFlow(flowId, summary);
setLastPublishedTitle(
publishedFlow?.data.alteredNodes
? `Successfully published changes to ${publishedFlow.data.alteredNodes.length} node(s)`
: "No new changes to publish"
);
}}
disabled={!alteredNodes || alteredNodes.length === 0}
>
PUBLISH
</Button>
</DialogActions>
</Dialog>
<Box mr={0}>
<Typography variant="caption">{lastPublishedTitle}</Typography>
</Box>
Expand Down
29 changes: 26 additions & 3 deletions editor.planx.uk/src/pages/FlowEditor/lib/store/editor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ import axios from "axios";
import { getCookie } from "lib/cookie";
import { client } from "lib/graphql";
import debounce from "lodash/debounce";
import isEmpty from "lodash/isEmpty";
import omitBy from "lodash/omitBy";
import type { FlowSettings, TextContent } from "types";
import type { GetState, SetState } from "zustand/vanilla";

Expand Down Expand Up @@ -56,6 +58,7 @@ export interface EditorStore extends Store.Store {
copyNode: (id: Store.nodeId) => void;
createFlow: (teamId: any, newSlug: any) => Promise<string>;
deleteFlow: (teamId: number, flowSlug: string) => Promise<object>;
diffFlow: (flowId: string) => Promise<any>;
getFlows: (teamId: number) => Promise<any>;
isClone: (id: Store.nodeId) => boolean;
lastPublished: (flowId: string) => Promise<string>;
Expand All @@ -68,7 +71,7 @@ export interface EditorStore extends Store.Store {
toParent?: Store.nodeId
) => void;
pasteNode: (toParent: Store.nodeId, toBefore: Store.nodeId) => void;
publishFlow: (flowId: string) => Promise<any>;
publishFlow: (flowId: string, summary?: string) => Promise<any>;
removeNode: (id: Store.nodeId, parent: Store.nodeId) => void;
updateFlowSettings: (
teamSlug: string,
Expand Down Expand Up @@ -211,6 +214,18 @@ export const editorStore = (
return response;
},

diffFlow(flowId: string) {
const token = getCookie("jwt");

return axios({
url: `${process.env.REACT_APP_API_URL}/flows/${flowId}/diff`,
method: "POST",
headers: {
Authorization: `Bearer ${token}`,
},
});
},
Copy link
Contributor

Choose a reason for hiding this comment

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

tiny nit and maybe a nit but idk -

  1. could we use axios.post() here for consistency

  2. I don't know if we should be returning this promise like this as not being handled in a try/catch by the caller, PreviewBrowser.tsx const alteredFlow = await diffFlow(flowId);, which could(?) lead to sad times?

I must admit I can sometimes get a bit lost in considering implications of async stuff though. I was considering adding something like https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/eslint-plugin/docs/rules/no-floating-promises.md to catch little edge cases like this. I think maybe the quickest fix here would be to (a) ignore this for now 😅 (b) handle the catch in the caller.

I'll be very happy to go along with whatever you think makes sense!

Copy link
Member Author

Choose a reason for hiding this comment

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

ah good thinking, this stuff is indeed tedious & tricky - i picked option b and added a try/catch to the caller to log any errors!


getFlows: async (teamId) => {
client.cache.reset();
const { data } = await client.query({
Expand Down Expand Up @@ -322,11 +337,19 @@ export const editorStore = (
}
},

publishFlow(flowId: string) {
publishFlow(flowId: string, summary?: string) {
const token = getCookie("jwt");

const urlWithParams = (url: string, params: any) =>
[url, new URLSearchParams(omitBy(params, isEmpty))]
.filter(Boolean)
.join("?");

return axios({
url: `${process.env.REACT_APP_API_URL}/flows/${flowId}/publish`,
url: urlWithParams(
`${process.env.REACT_APP_API_URL}/flows/${flowId}/publish`,
{ summary }
),
method: "POST",
headers: {
Authorization: `Bearer ${token}`,
Expand Down
7 changes: 4 additions & 3 deletions hasura.planx.uk/metadata/tables.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -161,11 +161,12 @@
- role: public
permission:
columns:
- id
- publisher_id
- data
- created_at
- data
- flow_id
- id
- publisher_id
- summary
jessicamcinchak marked this conversation as resolved.
Show resolved Hide resolved
filter: {}
- table:
schema: public
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
ALTER TABLE "public"."published_flows" DROP COLUMN "summary";
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
ALTER TABLE "public"."published_flows" ADD COLUMN "summary" text NULL;
Copy link
Contributor

Choose a reason for hiding this comment

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

while I'm looking at sql, maybe we need an index on published_flows.flow_id? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

added a hash index as I think we'll always do equality checks against flow_id !

also motivation to upgrade hasura: getting to create/manage indexes in the console rather than via manual sql command migrations!! hasura/graphql-engine#2219

Copy link
Contributor

Choose a reason for hiding this comment

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

ah nice, I think we'll be able to upgrade this week! According to them, the upgrade should just involve changing the version number in the dockerfile. We'll see...