Fls 1450 designer pre award integration #221
Conversation
israr-ulhaq
force-pushed
the
FLS-1450-designer-pre-award-integration
branch
3 times, most recently
from
3aab5f9 to
8f1f2ca
Compare
- Add PreAwardApiClient with TypeScript interfaces - Add preAwardApiUrl configuration with default value - Support for createOrUpdateForm, getAllForms, getFormDraft methods
- Extend existing API routes with Pre-Award API support - Update form save operations to use Pre-Award API - Add new form creation with Pre-Award API integration - Maintain backward compatibility with original routes
- Remove form cloning complexity - Direct navigation to form designer from Pre-Award API - Forms now edited directly instead of creating copies
- Add unit tests for PreAwardApiClient with error handling - Add integration tests for API routes - Configure Lab testing framework with TypeScript support - Add npm test scripts for development workflow
israr-ulhaq
force-pushed
the
FLS-1450-designer-pre-award-integration
branch
from
8f1f2ca to
8e254c8
Compare
|
There was a problem hiding this comment.
Brilliant stuff @israr-ulhaq 👏
The way you've organised the commits is honestly so helpful, and really made my life easier as a reviewer. And the code is great as well, this was not a straightforward task and you're 80% of the way there straight off the bat. I've left several comments which will need addressing, although I know you are a bit tied for time at the moment, so this might end up being a bit of a joint venture moving forward.
Random notes:
- We'll need a new Pre-Award PR to adjust the get multiple forms endpoint, as a precursor to this going out
- Keeping the call to Runner's
/publishendpoint intact means this PR can be deployed in isolation - nice call - We'll want to confirm a deployment plan before we do merge this anyway, synchronising across services and databases
| import config from "../config"; | ||
| import * as Wreck from "@hapi/wreck"; | ||
|
|
||
| interface FormJson { |
There was a problem hiding this comment.
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
|
|
||
| async getAllForms(): Promise<FormResponse[]> { | ||
| try { | ||
| const { payload: responseData } = await Wreck.get( |
There was a problem hiding this comment.
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`, | ||
| { | ||
| payload: JSON.stringify(payload), | ||
| headers: { |
There was a problem hiding this comment.
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
| } | ||
| } | ||
|
|
||
| async getAllForms(): Promise<FormResponse[]> { |
There was a problem hiding this comment.
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
| } | ||
| } | ||
|
|
||
| async getAllForms(): Promise<FormResponse[]> { |
There was a problem hiding this comment.
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
| is_published: boolean; | ||
| } | ||
|
|
||
| export class PreAwardApiClient { |
There was a problem hiding this comment.
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
| const parsedData = JSON.parse((responseData as Buffer).toString()); | ||
| return parsedData as FormResponse; | ||
| } | ||
| catch (error) { |
There was a problem hiding this comment.
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
| }; | ||
|
|
||
| // Extend the original putFormWithId with Pre-Award API support | ||
| export const putFormWithId: ServerRoute = { |
There was a problem hiding this comment.
This function isn't actually used, believe it or not! It's overwritten by the putFormWithIdRouteRegister stuff. We can just remove this completely
| import newFormJson from "../../../../digital-form-builder/designer/new-form.json"; | ||
|
|
||
| // Extend the original registerNewFormWithRunner with Pre-Award API support | ||
| export const registerNewFormWithRunner: ServerRoute = { |
There was a problem hiding this comment.
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
| authCookieName: joi.string().optional(), | ||
| sslKey: joi.string().optional(), | ||
| sslCert: joi.string().optional(), | ||
| preAwardApiUrl: joi.string().default("https://api.communities.gov.localhost:4004"), |
There was a problem hiding this comment.
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
|
We also need to add We should update the config variable name to match. |
|
We will also need to do this here:
See this Jira comment. |
https://mhclgdigital.atlassian.net/browse/FLS-1450
Summary
Integrates the Form Designer with Pre-Award API for centralised form storage, replacing the Form Runner (whichuse the Redis cache).
Changes Made
New Pre-Award API Integration
Core Functionality
Key Files Added/Modified
Testing
Run Unit Tests for Designer