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

Make run button functional #25

Merged
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
7d9c378
WIP 2: things related to post-form-submit, ie api and store
david-mears-2 Sep 3, 2024
02be36f
WIP. We dont have unit tests on the wip api endpoint + handler
david-mears-2 Sep 3, 2024
d1d4f9c
Remove store-related code
david-mears-2 Sep 4, 2024
7b146ba
test playwright ci
david-mears-2 Sep 4, 2024
52b12eb
Switch to using query params instead of body
david-mears-2 Sep 4, 2024
bcb53f2
Tests
david-mears-2 Sep 5, 2024
7434c9d
Refactor: Use pinia store for largeScreen refs
david-mears-2 Sep 5, 2024
782a8db
Update e2e test
david-mears-2 Sep 6, 2024
f45d8e9
remove comment
david-mears-2 Sep 6, 2024
12b7581
Try to improve playwright test
david-mears-2 Sep 6, 2024
8c0c4f5
Address PR comments
david-mears-2 Sep 10, 2024
0906bca
Use body instead of query for req's to run scenario
david-mears-2 Sep 10, 2024
0966d2b
Temporary change to allow web app to be compatible with latest r api
david-mears-2 Sep 10, 2024
143063c
Update e2e test to match latest R API build
david-mears-2 Sep 10, 2024
fbb2a05
Address PR comments
david-mears-2 Sep 10, 2024
2a03920
Use JSON data for request bodies instead of FormData
david-mears-2 Sep 12, 2024
67ba34d
fix integration tests
M-Kusumgar Sep 12, 2024
c3a4d8e
Merge pull request #30 from jameel-institute/jidea-57-non-country-inp…
david-mears-2 Sep 12, 2024
fbc712e
test changes
EmmaLRussell Sep 13, 2024
93ca120
parse body in test
EmmaLRussell Sep 13, 2024
501d92a
Merge pull request #33 from jameel-institute/jidea-57-mock-navigateTo-2
david-mears-2 Sep 13, 2024
d84c44c
Merge branch 'main' into jidea-57-non-country-inputs-rebasing-onto-fr…
david-mears-2 Sep 13, 2024
3e43583
Update e2e test to use real test run ids
david-mears-2 Sep 13, 2024
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: 14 additions & 12 deletions components/ParameterForm.vue
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
v-if="props.metadata && formData"
class="inputs"
role="form"
:data-test-form-data="JSON.stringify(formData)"
:data-test-navigate-to="navigateToData"
@submit.prevent="submitForm"
>
Expand All @@ -24,7 +23,7 @@
<CButtonGroup
role="group"
:aria-label="parameter.label"
:size="screenIsLarge ? 'lg' : undefined"
:size="appStore.largeScreen ? 'lg' : undefined"
>
<!-- This component's "v-model" prop type signature dictates we can't pass it a number. -->
<CFormCheck
Expand All @@ -38,7 +37,6 @@
autocomplete="off"
:label="option.label"
:value="option.id"
:disabled="!pageMounted"
/>
</CButtonGroup>
</CRow>
Expand All @@ -52,8 +50,7 @@
:id="parameter.id"
v-model="formData[parameter.id]"
:aria-label="parameter.label"
class="form-select" :class="[screenIsLarge ? 'form-select-lg' : '']"
:disabled="!pageMounted"
class="form-select" :class="[appStore.largeScreen ? 'form-select-lg' : '']"
>
<option
v-for="(option) in parameter.options"
Expand All @@ -69,9 +66,9 @@
<CButton
id="run-button"
color="primary"
:size="screenIsLarge ? 'lg' : undefined"
:size="appStore.largeScreen ? 'lg' : undefined"
type="submit"
:disabled="formSubmitting || !pageMounted"
:disabled="formSubmitting"
@click="submitForm"
>
Run
Expand Down Expand Up @@ -99,16 +96,18 @@
metadataFetchError: FetchError | null
}>();

// This is only a temporary piece of code, used until we implement numeric inputs.
const allParametersOfImplementedTypes = computed(() => props.metadata?.parameters.filter(({ parameterType }) => parameterType !== ParameterType.Numeric));

const formData = ref(
// Create a new object with keys set to the id values of the metadata.parameters array of objects, and all values set to default values.
props.metadata?.parameters.reduce((acc, { id, defaultOption, options }) => {
allParametersOfImplementedTypes.value?.reduce((acc, { id, defaultOption, options }) => {
acc[id] = defaultOption || options[0].id;
return acc;
}, {} as { [key: string]: string | number }),
);

const appStore = useAppStore();
const { screenIsLarge } = storeToRefs(appStore);
const navigateToData = ref("");
const pageMounted = ref(false);

Expand All @@ -135,12 +134,15 @@
return;
};

const formDataObject = new FormData();
Object.entries(formData.value).forEach(([key, value]) => {
formDataObject.append(key, value.toString());
});
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd assumed that we were just going to post JSON in the same format that the R API accepts and pass it straight through? You're saying that doesn't work (in the tests), that ordinary objects get turned into... what? Completely empty body?

It's not something to do with needing to set the content type is it? Does it default to POSTing form encoded data so it barfs on plain objects but accepts FormData? Hm, the ofetch docs suggest it should deal with that transparently and add the header itself, but it is supicious... Could try setting that header explicitly?

Copy link
Contributor Author

@david-mears-2 david-mears-2 Sep 12, 2024

Choose a reason for hiding this comment

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

I have found something that appears to be working - JSON stringifying the body (in the test). The oftech docs claimed that this happened automatically, but evidently not 😕

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems that still it behaves differently under test than in real life, so I had to add in a check for whether the body was being received as an object (as in real life, and in e2e tests) or a JSON parsable string (as in the integration test). I hope this is tolerable, as the price for moving away from FormData.

Copy link
Contributor

Choose a reason for hiding this comment

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

As discussed. maybe try adding header Content-Type: application/json when doing the POST.


formSubmitting.value = true;
const response = await $fetch<NewScenarioData>("/api/scenarios", {
Copy link
Contributor

Choose a reason for hiding this comment

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

POST "scenarios" seems a bit odd when it's only posting a single scenario - or is the idea that it will do multiple scenarios when we do the comparison feature? (In that case it will be POSTing an array of parameters potentially..)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was imagining all of the scenario-related endpoints living under a single 'scenarios' namespace, rather than varying between 'scenario' and 'scenarios' depending on the use case. I believe in CRUD it is conventional to standardize on the plural form in route names.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, it's a bit uneasy. Fair enough!

method: "POST",
query: { // Using query instead of body because I couldn't work out how to send a body in the integration test.
parameters: formData.value,
},
body: formDataObject,

Check warning on line 145 in components/ParameterForm.vue

View check run for this annotation

Codecov / codecov/patch

components/ParameterForm.vue#L145

Added line #L145 was not covered by tests
}).catch((error: FetchError) => {
console.error(error);
absternator marked this conversation as resolved.
Show resolved Hide resolved
});
Expand All @@ -153,7 +155,7 @@
};

onMounted(() => {
pageMounted.value = true;

Check warning on line 158 in components/ParameterForm.vue

View check run for this annotation

Codecov / codecov/patch

components/ParameterForm.vue#L158

Added line #L158 was not covered by tests
});
</script>

Expand Down
9 changes: 4 additions & 5 deletions components/SideBar.vue
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
<template>
<CSidebar
:visible="visible"
:unfoldable="screenIsLarge"
:overlaid="screenIsLarge"
:unfoldable="appStore.largeScreen"
:overlaid="appStore.largeScreen"
class="sidebar-fixed border-end"
@hide="handleHide"
>
<CSidebarNav role="navigation" data-testid="sidebar">
<CSidebarNav role="navigation">
<CNavItem>
<NuxtLink prefetch-on="interaction" to="/scenarios/new" class="nav-link">
<CIcon icon="cilPlus" size="lg" class="nav-icon" /> New scenario
Expand Down Expand Up @@ -72,10 +72,9 @@ const versionTooltipContent = computed(() => {
const visible = defineModel("visible", { type: Boolean, required: true });

const appStore = useAppStore();
const { screenIsLarge } = storeToRefs(appStore);

const resetSidebarPerScreenSize = () => {
visible.value = screenIsLarge.value;
visible.value = appStore.largeScreen;
};

const hideHasNeverBeenEmitted = ref(true);
Expand Down
5 changes: 2 additions & 3 deletions layouts/default.vue
Original file line number Diff line number Diff line change
Expand Up @@ -25,21 +25,20 @@
}

const appStore = useAppStore(useNuxtApp().$pinia);
appStore.initializeAppState();

const setScreenSize = () => {
// As this function uses window, it can only be run client-side, so we have to pass the $pinia instance
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// As this function uses window, it can only be run client-side, so we have to pass the $pinia instance
// As this function uses window, it can only be run client-side (post-setup), so we have to pass the $pinia instance

The linkage between these clauses wasn't immediately obvious to me!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went through an adventure since writing this, submitted a PR to clarify the docs of the nuxt pinia module, and eventually the maintainer let me know that in fact "you don't need to pass the pinia instance within setup"
vuejs/pinia#2766

As such, the next PR removes this, here: https://github.com/jameel-institute/daedalus-web-app/pull/27/files#diff-433a9abd4ca70520f69fc064c160bb093918ce26dda6087a8bbfdbe2895d1944

// to the useAppStore function: https://pinia.vuejs.org/ssr/nuxt.html#Awaiting-for-actions-in-pages
const breakpoint = 992; // CoreUI's "lg" breakpoint
if (window.innerWidth < breakpoint) {
appStore.setScreenSize(false);
appStore.largeScreen = false;
} else {
appStore.setScreenSize(true);
appStore.largeScreen = true;
}
};

onMounted(() => {
setScreenSize();

Check warning on line 41 in layouts/default.vue

View check run for this annotation

Codecov / codecov/patch

layouts/default.vue#L40-L41

Added lines #L40 - L41 were not covered by tests
window.addEventListener("resize", setScreenSize);
});
onBeforeUnmount(() => {
Expand Down
2 changes: 1 addition & 1 deletion playwright.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ export default defineConfig({
webServer: {
command: "npm run build && cd .output && node ./server/index.mjs",
url: "http://127.0.0.1:3000",
timeout: 120 * 1000,
timeout: 30 * 1000,
reuseExistingServer: true,
stdout: "pipe", // Debugging
},
Expand Down
12 changes: 4 additions & 8 deletions server/api/scenarios.post.ts
Original file line number Diff line number Diff line change
@@ -1,16 +1,12 @@
import { runScenario } from "@/server/handlers/scenarios";
import { defineRApiEventHandler } from "~/server/utils/defineRApiEventHandler";
import { defineRApiEventHandler } from "@/server/utils/defineRApiEventHandler";
import { formDataToObject } from "@/server/utils/helpers";

Check warning on line 3 in server/api/scenarios.post.ts

View check run for this annotation

Codecov / codecov/patch

server/api/scenarios.post.ts#L2-L3

Added lines #L2 - L3 were not covered by tests
import type { NewScenarioResponse } from "@/types/apiResponseTypes";
import type { ParameterDict } from "@/types/apiRequestTypes";

export default defineRApiEventHandler(
async (event): Promise<NewScenarioResponse> => {
const query = getQuery(event);

const modelParameters = JSON.parse(query.parameters as string) as ParameterDict;

const newScenarioResponse = await runScenario(modelParameters, event);

const formDataBody = await readFormData(event);
const newScenarioResponse = await runScenario(formDataToObject(formDataBody), event);
return newScenarioResponse;
},
);

Check warning on line 12 in server/api/scenarios.post.ts

View check run for this annotation

Codecov / codecov/patch

server/api/scenarios.post.ts#L6-L12

Added lines #L6 - L12 were not covered by tests
16 changes: 16 additions & 0 deletions server/utils/helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,19 @@ export const errorMessage = (errors: Array<ApiError> | null) => {
return [apiError.error, apiError.detail].join(": ");
}).join(", ");
};

// Convert FormData to Object. It is designed to be able to cope with multiselects and checkboxes. https://stackoverflow.com/a/46774073
export const formDataToObject = (formData: FormData) => {
const object: { [key: string]: any } = {};
formData.forEach((value, key) => {
if (!Reflect.has(object, key)) {
object[key] = value;
return;
}
if (!Array.isArray(object[key])) {
object[key] = [object[key]];
}
object[key].push(value);
});
return object;
};
13 changes: 0 additions & 13 deletions stores/appStore.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,17 +5,4 @@ export const useAppStore = defineStore("app", {
state: (): AppState => ({
largeScreen: true,
}),
getters: {
screenIsLarge: (state: AppState): boolean => {
return state.largeScreen;
},
},
actions: {
initializeAppState(): void {
this.largeScreen = true;
},
setScreenSize(large: boolean): void {
this.largeScreen = large;
},
},
});
6 changes: 2 additions & 4 deletions tests/e2e/runAnalysis.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,12 @@ test("Can request a scenario analysis run", async ({ page, baseURL }) => {
await page.waitForURL(`${baseURL}/scenarios/new`);

await expect(page.getByText("Simulate a new scenario")).toBeVisible();
await expect(page.getByRole("form")).toBeVisible({ timeout: 30000 });
await expect(page.getByRole("form")).toBeVisible();

await page.selectOption('select[id="pathogen"]', { label: "Influenza 1957" });
await page.selectOption('select[id="response"]', { label: "Elimination" });
await page.selectOption('select[id="country"]', { label: "United States" });
await page.click('div[aria-label="Advance vaccine investment"] label[for="low"]');

await expect(page.getByRole("form")).toHaveAttribute("data-test-form-data", "{\"country\":\"United States\",\"pathogen\":\"influenza-1957\",\"response\":\"elimination\",\"vaccine\":\"low\"}");
await page.click('div[aria-label="Global vaccine investment"] label[for="low"]');

await page.click('button:has-text("Run")');

Expand Down
33 changes: 24 additions & 9 deletions tests/integration/rApi.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -123,12 +123,17 @@ describe("endpoints which consume the R API", { sequential: true }, async () =>

// In these tests, Mockoon is configured to check the request body for all the expected parameters (and respond with the
// appropriate status code etc.), as a way to test that the parameters (and model version) are being passed through all the way
// to the R API.
// to the R API. This is called a 'rule' in Mockoon, and rules can't be used simultaneously with the 'sequential' setting, so we
// instead use the mockoonResponse parameter to tell Mockoon which type of response to send.
describe("post api/scenarios", async () => {
it("returns a successful response when the mock server responds successfully", async () => {
const queryString = `parameters={"mockoonResponse":"successful","country":"Thailand","pathogen":"sars-cov-1","response":"no_closure","vaccine":"none"}`;

const response = await nuxtTestUtilsFetch(`/api/scenarios?${queryString}`, { method: "POST" });
const formData = new FormData();
formData.append("mockoonResponse", "successful");
formData.append("country", "Thailand");
formData.append("pathogen", "sars-cov-1");
formData.append("response", "no_closure");
formData.append("vaccine", "none");
const response = await nuxtTestUtilsFetch(`/api/scenarios`, { method: "POST", body: formData });

expect(response.ok).toBe(true);
expect(response.status).toBe(200);
Expand All @@ -139,9 +144,14 @@ describe("endpoints which consume the R API", { sequential: true }, async () =>
});

it("returns a response with informative errors when the mock server responds with an error", async () => {
const queryString = `parameters={"mockoonResponse":"notFound","country":"Thailand","pathogen":"sars-cov-1","response":"no_closure","vaccine":"none"}`;
const formData = new FormData();
formData.append("mockoonResponse", "notFound");
formData.append("country", "Thailand");
formData.append("pathogen", "sars-cov-1");
formData.append("response", "no_closure");
formData.append("vaccine", "none");

const response = await nuxtTestUtilsFetch(`/api/scenarios?${queryString}`, { method: "POST" });
const response = await nuxtTestUtilsFetch(`/api/scenarios`, { method: "POST", body: formData });

expect(response.ok).toBe(false);
expect(response.status).toBe(404);
Expand All @@ -153,9 +163,14 @@ describe("endpoints which consume the R API", { sequential: true }, async () =>
});

it("returns a response with informative errors when the mock server doesn't respond in time", async () => {
const queryString = `parameters={"mockoonResponse":"delayed","country":"Thailand","pathogen":"sars-cov-1","response":"no_closure","vaccine":"none"}`;

const response = await nuxtTestUtilsFetch(`/api/scenarios?${queryString}`, { method: "POST" });
const formData = new FormData();
formData.append("mockoonResponse", "delayed");
formData.append("country", "Thailand");
formData.append("pathogen", "sars-cov-1");
formData.append("response", "no_closure");
formData.append("vaccine", "none");

const response = await nuxtTestUtilsFetch(`/api/scenarios`, { method: "POST", body: formData });

expect(response.ok).toBe(false);
expect(response.status).toBe(500);
Expand Down
43 changes: 5 additions & 38 deletions tests/unit/components/ParameterForm.spec.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
import { describe, expect, it } from "vitest";
import { mountSuspended, registerEndpoint } from "@nuxt/test-utils/runtime";
import { getQuery } from "h3";
import { FetchError } from "ofetch";
import { flushPromises } from "@vue/test-utils";

import { formDataToObject } from "@/server/utils/helpers";
import type { Metadata } from "@/types/apiResponseTypes";
import ParameterForm from "@/components/ParameterForm.vue";

Expand Down Expand Up @@ -105,47 +106,13 @@ describe("parameter form", () => {
]);
});

it("initialises formData with defaults and updates formData when a parameter is changed", async () => {
const component = await mountSuspended(ParameterForm, {
props: { metadata, metadataFetchStatus: "success", metadataFetchError: null },
global: { stubs },
});

const cForm = component.findComponent({ name: "CForm" });
let formData = JSON.parse(cForm.element.attributes.getNamedItem("data-test-form-data")!.value);
expect(formData.region).toBe("HVN");
expect(formData.long_list).toBe("1");
expect(formData.short_list).toBe("no");

const selectElements = component.findAll("select");
const longListDropDown = selectElements[0];
const countrySelect = selectElements[1];

// Verify that the select elements are the ones we think they are
selectElements.forEach((selectElement, index) => {
const correctLabel = ["Drop Down", "Region"][index];
const paramId = selectElement.element.attributes.getNamedItem("id")!.value;
expect(component.find(`label[for=${paramId}]`).element.textContent).toBe(correctLabel);
});

await longListDropDown.findAll("option").at(2)!.setSelected();
await countrySelect.findAll("option").at(0)!.setSelected();
await component.findComponent({ name: "CButtonGroup" }).find("input[value='yes']").setChecked();

formData = JSON.parse(cForm.element.attributes.getNamedItem("data-test-form-data")!.value);
expect(formData.region).toBe("CLD");
expect(formData.long_list).toBe("3");
expect(formData.short_list).toBe("yes");
});

it("sends a POST request to /api/scenarios with the form data when submitted", async () => {
registerEndpoint("/api/scenarios", {
method: "POST",
handler: (event) => {
const query = getQuery(event) as Record<string, string>;
const modelParams = JSON.parse(query.parameters);
async handler(event) {
const parameters = formDataToObject(event.node.req.body) as Record<string, string>;

if (modelParams.long_list === "1" && modelParams.region === "HVN" && modelParams.short_list === "no") {
if (parameters.long_list === "1" && parameters.region === "HVN" && parameters.short_list === "no") {
return { runId: "randomId" };
} else {
return { error: "Test failed due to wrong parameters" };
Expand Down
28 changes: 28 additions & 0 deletions tests/unit/server/utils/helpers.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
import { describe, expect, it } from "vitest";

import { formDataToObject } from "@/server/utils/helpers";

describe("formDataToObject", () => {
it("should convert FormData to an object with correct values, handling single and multiple values for the same key correctly", () => {
const formData = new FormData();
formData.append("name", "John Doe");
formData.append("age", "30");
formData.append("hobby", "reading");
formData.append("hobby", "coding");

const expectedObject = {
name: "John Doe",
age: "30",
hobby: ["reading", "coding"],
};

expect(formDataToObject(formData)).toEqual(expectedObject);
});

it("should return an empty object for empty FormData", () => {
const formData = new FormData();
const expectedObject = {};

expect(formDataToObject(formData)).toEqual(expectedObject);
});
});
10 changes: 0 additions & 10 deletions tests/unit/stores/appStore.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,16 +11,6 @@ describe("app store", () => {
it("initialises correctly", async () => {
const store = useAppStore();
expect(store.largeScreen).toBe(true);
store.initializeAppState();
expect(store.largeScreen).toBe(true);
});

it("can update and retrieve the screen size", async () => {
const store = useAppStore();
const { screenIsLarge } = storeToRefs(store);

store.setScreenSize(false);
expect(screenIsLarge.value).toBe(false);
});
});
});
1 change: 1 addition & 0 deletions types/apiResponseTypes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ export interface ParameterOption {
export enum ParameterType {
Select = "select",
GlobeSelect = "globeSelect",
Numeric = "numeric",
}
export interface Parameter {
id: string
Expand Down
Loading