Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
26 changes: 10 additions & 16 deletions designer/client/pages/landing-page/ViewFundForms.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ type Props = {
type State = {
configs: { Key: string; DisplayName: string }[];
loading?: boolean;

};

export class ViewFundForms extends Component<Props, State> {
Expand All @@ -25,30 +26,23 @@ export class ViewFundForms extends Component<Props, State> {
};
}

componentDidMount() {
formConfigurationApi.loadConfigurations().then((configs) => {
async componentDidMount() {
try {
const configs = await formConfigurationApi.loadConfigurations();
this.setState({
loading: false,
configs,
});
});
} catch (error) {
logger.error("ViewFundForms componentDidMount", error);
this.setState({ loading: false });
}
}

selectForm = async (form) => {
try {
const response = await window.fetch("/api/new", {
method: "POST",
body: JSON.stringify({
selected: {Key: form},
name: "",
}),
headers: {
Accept: "application/json",
"Content-Type": "application/json",
},
});
const responseJson = await response.json();
this.props.history.push(`/designer/${responseJson.id}`);
// Always go directly to edit the form from Pre-Award API
this.props.history.push(`/designer/${form}`);
} catch (e) {
logger.error("ChooseExisting", e);
}
Expand Down
4 changes: 3 additions & 1 deletion designer/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,9 @@
"static-content:dist-push": "cp -r ../designer/public/static/images dist/client/assets && cp -r ../designer/public/static/css dist/client/assets",
"build": "NODE_ENV=production && NODE_OPTIONS=--openssl-legacy-provider && webpack && yarn run static-content:dist-push",
"start:server": "node dist/server.js",
"start:local": "NODE_ENV=development PERSISTENT_BACKEND=preview ts-node-dev --inspect=0.0.0.0:9229 --respawn --transpile-only server/index.ts"
"start:local": "NODE_ENV=development PERSISTENT_BACKEND=preview ts-node-dev --inspect=0.0.0.0:9229 --respawn --transpile-only server/index.ts",
"test": "lab test/cases/server/plugins/*.test.ts -T test/.transform.js -v",
"unit-test": "lab test/cases/server/plugins/*.test.ts -T test/.transform.js -v -S -r console -o stdout -r html -o unit-test.html -I version -l"
},
"author": "Communities UK",
"license": "SEE LICENSE IN LICENSE",
Expand Down
3 changes: 3 additions & 0 deletions designer/server/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ export interface Config {
authCookieName: string,
sslKey: string,
sslCert: string,
preAwardApiUrl: string,
}

// server-side storage expiration - defaults to 20 minutes
Expand Down Expand Up @@ -65,6 +66,7 @@ const schema = joi.object({
authCookieName: joi.string().optional(),
sslKey: joi.string().optional(),
sslCert: joi.string().optional(),
preAwardApiUrl: joi.string().default("https://api.communities.gov.localhost:4004"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd like to see the path /forms added to the preAwardApiUrl throughout - this path is part of all URLs we hit, and so we should share it for simplicity, plus it's the way I've done it in Runner, so it'll be consistent

});

// Build config
Expand All @@ -90,6 +92,7 @@ const config = {
authCookieName: process.env.AUTH_COOKIE_NAME,
sslKey: process.env.SSL_KEY,
sslCert: process.env.SSL_CERT,
preAwardApiUrl: process.env.PRE_AWARD_API_URL || "https://api.communities.gov.localhost:4004",
};

// Validate config
Expand Down
100 changes: 100 additions & 0 deletions designer/server/lib/preAwardApiClient.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,100 @@
import config from "../config";
import * as Wreck from "@hapi/wreck";

interface FormJson {
Copy link
Collaborator

Choose a reason for hiding this comment

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

My instinct would be that this isn't needed - best to just treat the form JSON as a black box, a bit like how the Pre-Award API itself does - let's just build a barren interface and let the downstream Designer internals figure out what to do with this object

startPage: string;
pages: any[];
sections: any[];
name: string;
version?: number;
conditions?: any[];
lists?: any[];
metadata?: any;
fees?: any[];
outputs?: any[];
skipSummary?: boolean;
[key: string]: any;
}

export interface FormData {
name: string;
form_json: FormJson;
}

export interface FormResponse {
id: string;
name: string;
created_at: string;
updated_at: string;
published_at: string | null;
draft_json: FormJson;
published_json: FormJson;
is_published: boolean;
}

export class PreAwardApiClient {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I noticed you didn't include any auth headers in any of the requests to the Pre-Award Forms API, and initially I was confused why that wasn't breaking things - I thought I'd added auth protection to the API endpoints. This led to some digging, and it turns out that regardless of the fact that adding decorators = [login_required] to my view classes didn't do what I expected it to, we don't actually have any auth protection on any of the Pre-Award APIs anyway - not just the Forms API. Instead, we rely on network-level authentication, which is to say that the APIs are only exposed within the AWS VPC network. So I can remove all of the gubbins in my Runner PR around auth headers! Live and learn eh

private baseUrl: string;

constructor() {
this.baseUrl = config.preAwardApiUrl;
}

async createOrUpdateForm(formData: FormData): Promise<FormResponse>{
const payload = formData;

try{
const { payload: responseData } = await Wreck.post(
`${this.baseUrl}/forms`,
{
payload: JSON.stringify(payload),
headers: {
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you use the wreck.defaults function in the constructor, you can set the Content-Type globally to application/json, reducing repetition and making function implementations simpler

'Content-Type': 'application/json'
}
}
);
const parsedData = JSON.parse((responseData as Buffer).toString());
return parsedData as FormResponse;
}
catch (error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Catching an error just to throw it again with no side effects is a redundant behaviour, we might as well just remove the try / catch syntax here completely, same goes for the other functions

throw error;
}
}

async getAllForms(): Promise<FormResponse[]> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This takes ABSOLUTELY AGES if you have lots of forms. I've got 338 forms in my local Pre-Award database and I think I was waiting 7 or 8 seconds for the /app/existing-forms page to load. The reason for this is the sheer volume of bytes being sent over the network - currently the Pre-Award API returns every single attribute of a form, including most pertinently draft_json and published_json, each of which can be massive in its own right. By returning just the things we need from the API - in this case name and updated_at, speed stops being catastrophic. Obviously the API is a funding-service-pre-award thing, so we'll need a separate PR over there, but we should spin one up tout suite! We'd then need to adjust the FormResponse interface on this end

Copy link
Collaborator

Choose a reason for hiding this comment

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

Separate thing - but I noticed some weird behaviour whereby forms get repositioned in the existing forms list after you've edited them. Would be good to avoid that, as it makes you feel you've gone a bit mad as a user

try {
const { payload: responseData } = await Wreck.get(
Copy link
Collaborator

Choose a reason for hiding this comment

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

I actually get an error running this code locally: Wreck.get is not a function. If I do this instead:

...
import wreck from "@hapi/wreck";
...
export class PreAwardApiClient {
...
   constructor() {
      ...
      this.wreck = wreck.defaults();
   }
   ...
   async getAllForms(): Promise<FormResponse[]> {
      ...
      try {
         const { payload: responseData } = await this.wreck.get(
            ...
         )
      ...
      }
   ...
   }
...
}

It works for me. I guess it's the same with Wreck.post. If it is working for you, would be interesting to know why, although I suspect that'd lead us down a rabbit hole 🐰

`${this.baseUrl}/forms`,
{
headers: {
'Content-Type': 'application/json'
}
}
);

const parsedData = JSON.parse((responseData as Buffer).toString());
return parsedData as FormResponse[];
} catch (error) {
throw error;
}
}

async getFormDraft(name: string): Promise<FormJson>{
try{
const { payload: responseData } = await Wreck.get(
`${this.baseUrl}/forms/${name}/draft`,
{
headers: {
'Content-Type': 'application/json'
}
}
);
const parsedData = JSON.parse((responseData as Buffer).toString());
return parsedData as FormJson;
}
catch (error) {
throw error;
}
}
}

export const preAwardApiClient = new PreAwardApiClient();
21 changes: 12 additions & 9 deletions designer/server/plugins/DesignerRouteRegister.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
import {newConfig, api, app} from "../../../digital-form-builder/designer/server/plugins/routes";
import {app} from "../../../digital-form-builder/designer/server/plugins/routes";
import {envStore, flagg} from "flagg";
import {putFormWithIdRouteRegister} from "./routes/PutFormWithIdRouteRegister";
import {registerNewFormWithRunner} from "./routes/newConfig";
import {getFormWithId, getAllPersistedConfigurations, log} from "./routes/api";
import config from "../config";
import {jwtAuthStrategyName} from "./AuthPlugin";

Expand Down Expand Up @@ -83,15 +85,15 @@ export const designerPlugin = {
// @ts-ignore
app.redirectOldUrlToDesigner.options.auth = jwtAuthStrategyName
// @ts-ignore
newConfig.registerNewFormWithRunner.options.auth = jwtAuthStrategyName
registerNewFormWithRunner.options.auth = jwtAuthStrategyName
// @ts-ignore
api.getFormWithId.options.auth = jwtAuthStrategyName
getFormWithId.options.auth = jwtAuthStrategyName
// @ts-ignore
putFormWithIdRouteRegister.options.auth = jwtAuthStrategyName
// @ts-ignore
api.getAllPersistedConfigurations.options.auth = jwtAuthStrategyName
getAllPersistedConfigurations.options.auth = jwtAuthStrategyName
// @ts-ignore
api.log.options.auth = jwtAuthStrategyName
log.options.auth = jwtAuthStrategyName
}

server.route(startRoute);
Expand All @@ -118,6 +120,7 @@ export const designerPlugin = {
store: envStore(process.env),
definitions: {
featureEditPageDuplicateButton: {default: false},

},
});

Expand All @@ -128,11 +131,11 @@ export const designerPlugin = {
},
});

server.route(newConfig.registerNewFormWithRunner);
server.route(api.getFormWithId);
server.route(registerNewFormWithRunner);
server.route(getFormWithId);
server.route(putFormWithIdRouteRegister);
server.route(api.getAllPersistedConfigurations);
server.route(api.log);
server.route(getAllPersistedConfigurations);
server.route(log);
},
},
};
6 changes: 6 additions & 0 deletions designer/server/plugins/routes/PutFormWithIdRouteRegister.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
import {ServerRoute} from "@hapi/hapi";
import {AdapterSchema} from "@communitiesuk/model";
import {publish} from "../../../../digital-form-builder/designer/server/lib/publish";
import {preAwardApiClient} from "../../lib/preAwardApiClient";
import config from "../../config";


export const putFormWithIdRouteRegister: ServerRoute = {
Expand Down Expand Up @@ -31,6 +33,10 @@ export const putFormWithIdRouteRegister: ServerRoute = {
`${id}`,
JSON.stringify(value)
);
// Save to Pre-Award API
const formData = { name: id, form_json: value };
await preAwardApiClient.createOrUpdateForm(formData);
// Publish to runner for preview
await publish(id, value);
return h.response({ok: true}).code(204);
} catch (err) {
Expand Down
60 changes: 60 additions & 0 deletions designer/server/plugins/routes/api.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
import { api as originalApi } from "../../../../digital-form-builder/designer/server/plugins/routes";
import { preAwardApiClient } from "../../lib/preAwardApiClient";
import config from "../../config";
import { ServerRoute, ResponseObject } from "@hapi/hapi";

// Extend the original getFormWithId with Pre-Award API support
export const getFormWithId: ServerRoute = {
...originalApi.getFormWithId,
options: {
...originalApi.getFormWithId.options || {},
handler: async (request, h) => {
const { id } = request.params;
const formJson = await preAwardApiClient.getFormDraft(id);
return h.response(formJson).type("application/json");
},
},
};

// Extend the original putFormWithId with Pre-Award API support
export const putFormWithId: ServerRoute = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This function isn't actually used, believe it or not! It's overwritten by the putFormWithIdRouteRegister stuff. We can just remove this completely

...originalApi.putFormWithId,
options: {
...originalApi.putFormWithId.options || {},
handler: async (request, h) => {
const { id } = request.params;
const { Schema } = await import("../../../../digital-form-builder/model/src");
const { value, error } = Schema.validate(request.payload, {
abortEarly: false,
});

if (error) {
throw new Error("Schema validation failed, reason: " + error.message);
}
const formData = { name: id, form_json: value };
await preAwardApiClient.createOrUpdateForm(formData);

return h.response({ ok: true }).code(204);
},
},
};

// Extend the original getAllPersistedConfigurations with Pre-Award API support
export const getAllPersistedConfigurations: ServerRoute = {
...originalApi.getAllPersistedConfigurations,
options: {
...originalApi.getAllPersistedConfigurations.options || {},
handler: async (request, h): Promise<ResponseObject | undefined> => {
const forms = await preAwardApiClient.getAllForms();
const response = forms.map(form => ({
Key: form.name,
DisplayName: form.name,
LastModified: form.updated_at
}));
return h.response(response).type("application/json");
},
},
};

// Use original log route as-is
export const log = originalApi.log;
42 changes: 42 additions & 0 deletions designer/server/plugins/routes/newConfig.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
import { newConfig as originalNewConfig } from "../../../../digital-form-builder/designer/server/plugins/routes";
import { preAwardApiClient } from "../../lib/preAwardApiClient";
import config from "../../config";
import { ServerRoute } from "@hapi/hapi";
import { HapiRequest } from "../../../../digital-form-builder/designer/server/types";
import { nanoid } from "nanoid";
import newFormJson from "../../../../digital-form-builder/designer/new-form.json";

// Extend the original registerNewFormWithRunner with Pre-Award API support
export const registerNewFormWithRunner: ServerRoute = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we need both this and putFormWithIdRouteRegister - both are effectively doing the same thing - wrapping a call to the POST /forms endpoint in the Pre-Award API. The key thing to realise is that that endpoint does not care whether you're creating or updating a form, it simply takes a form name and the JSON definition and upserts. Please double check I'm right, and if I am remove this function and have new form creation hit putFormWithIdRouteRegister

...originalNewConfig.registerNewFormWithRunner,
options: {
...originalNewConfig.registerNewFormWithRunner.options,
handler: async (request: HapiRequest, h) => {
const { selected, name } = request.payload as any;

if (name && name !== "" && !name.match(/^[a-zA-Z0-9 _-]+$/)) {
return h
.response("Form name should not contain special characters")
.type("application/json")
.code(400);
}

const newName = name === "" ? nanoid(10) : name;

if (selected.Key === "New") {
const formData = { name: newName, form_json: newFormJson };
await preAwardApiClient.createOrUpdateForm(formData);
} else {
const existingForm = await preAwardApiClient.getFormDraft(selected.Key);
const formData = { name: newName, form_json: existingForm };
await preAwardApiClient.createOrUpdateForm(formData);
}

const response = JSON.stringify({
id: `${newName}`,
previewUrl: config.previewUrl,
});
return h.response(response).type("application/json").code(200);
},
},
};
17 changes: 17 additions & 0 deletions designer/test/.transform.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
const Babel = require("@babel/core");

module.exports = [
{
ext: ".ts",
transform: (content, filename) => {
const result = Babel.transformSync(content, {
filename,
presets: [
["@babel/preset-env", { targets: { node: "current" } }],
"@babel/preset-typescript"
]
});
return result.code;
}
}
];
Loading
Loading