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 21 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
62 changes: 36 additions & 26 deletions components/ParameterForm.vue
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
<template>
<div>
<div v-show="pageMounted">
absternator marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this actually necessary? A Playwright timing issue? It should be possible to write Playwright tests so that they wait for a condition to become true. I think I'd rather have messy tests than somewhat inexplicable properties of the code that are only there because of test behaviour. If it really isn't then let's have a comment explaining it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Luckily, this problem disappears next PR, because by then we can wait for version data as our signal that everything is mounted.

<CForm
v-if="props.metadata && formData"
class="inputs"
:data-test="JSON.stringify(formData)"
role="form"
@submit.prevent="submitForm"
>
<div
Expand All @@ -22,7 +22,7 @@
<CButtonGroup
role="group"
:aria-label="parameter.label"
:size="largeScreen ? '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 @@ -49,7 +49,7 @@
:id="parameter.id"
v-model="formData[parameter.id]"
:aria-label="parameter.label"
class="form-select" :class="[largeScreen ? 'form-select-lg' : '']"
class="form-select" :class="[appStore.largeScreen ? 'form-select-lg' : '']"
>
<option
v-for="(option) in parameter.options"
Expand All @@ -65,13 +65,14 @@
<CButton
id="run-button"
color="primary"
:size="largeScreen ? 'lg' : undefined"
:size="appStore.largeScreen ? 'lg' : undefined"
type="submit"
:disabled="formSubmitting || props.metadataFetchStatus === 'error'"
@click="submitForm"
>
Run
<CIcon
icon="cilArrowRight"
/>
<CSpinner v-if="formSubmitting && props.metadataFetchStatus !== 'error'" size="sm" class="ms-1" />
<CIcon v-else icon="cilArrowRight" />
</CButton>
</CForm>
<CAlert v-else-if="props.metadataFetchStatus === 'error'" color="warning">
Expand All @@ -84,8 +85,8 @@
<script lang="ts" setup>
import type { FetchError } from "ofetch";
import { CIcon } from "@coreui/icons-vue";
import type { Metadata, Parameter } from "@/types/daedalusApiResponseTypes";
import { ParameterType } from "@/types/daedalusApiResponseTypes";
import { ParameterType } from "@/types/apiResponseTypes";
import type { Metadata, NewScenarioData, Parameter } from "@/types/apiResponseTypes";
import type { AsyncDataRequestStatus } from "#app";

const props = defineProps<{
Expand All @@ -94,14 +95,20 @@
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 pageMounted = ref(false);

const optionsAreTerse = (parameter: Parameter) => {
const eachOptionIsASingleWord = parameter.options.every((option) => {
return !option.label.includes(" ");
Expand All @@ -118,26 +125,29 @@
return parameter.parameterType === ParameterType.Select && optionsAreTerse(parameter);
};

const submitForm = () => {
// Not implemented yet
};
const formSubmitting = ref(false);

const submitForm = async () => {
if (!formData.value) {
absternator marked this conversation as resolved.
Show resolved Hide resolved
return;
};

const largeScreen = ref(true);
const breakpoint = 992; // CoreUI's "lg" breakpoint
const setFieldSizes = () => {
if (window.innerWidth < breakpoint) {
largeScreen.value = false;
} else {
largeScreen.value = true;
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",
body: { parameters: formData.value },
}).catch((error: FetchError) => {
console.error(error);
absternator marked this conversation as resolved.
Show resolved Hide resolved
});

if (response) {
const { runId } = response;

Check warning on line 144 in components/ParameterForm.vue

View check run for this annotation

Codecov / codecov/patch

components/ParameterForm.vue#L143-L144

Added lines #L143 - L144 were not covered by tests
await navigateTo(`/scenarios/${runId}`);
}
};

onMounted(() => {
setFieldSizes();
window.addEventListener("resize", setFieldSizes);
});
onBeforeUnmount(() => {
window.removeEventListener("resize", setFieldSizes);
pageMounted.value = true;
});
</script>

Expand Down
19 changes: 6 additions & 13 deletions components/SideBar.vue
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
<template>
<CSidebar
:visible="visible"
:unfoldable="largeScreen"
:overlaid="largeScreen"
:unfoldable="appStore.largeScreen"
:overlaid="appStore.largeScreen"
class="sidebar-fixed border-end"
@hide="handleHide"
>
Expand Down Expand Up @@ -57,7 +57,7 @@

<script lang="ts" setup>
import { CIcon } from "@coreui/icons-vue";
import type { VersionData } from "@/types/daedalusApiResponseTypes";
import type { VersionData } from "@/types/apiResponseTypes";

const { data: versionData } = useFetch("/api/versions") as { data: Ref<VersionData> };

Expand All @@ -70,18 +70,11 @@ const versionTooltipContent = computed(() => {
});

const visible = defineModel("visible", { type: Boolean, required: true });
const largeScreen = ref(true);

const breakpoint = 992; // CoreUI's "lg" breakpoint
const appStore = useAppStore();

const resetSidebarPerScreenSize = () => {
// Set the default values for the sidebar based on the screen size.
if (window.innerWidth < breakpoint) {
visible.value = false;
largeScreen.value = false;
} else {
visible.value = true;
largeScreen.value = true;
}
visible.value = appStore.largeScreen;
};

const hideHasNeverBeenEmitted = ref(true);
Expand Down
21 changes: 21 additions & 0 deletions layouts/default.vue
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,27 @@
function handleToggleSidebarVisibility() {
sidebarVisible.value = !sidebarVisible.value;
}

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

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.largeScreen = false;
} else {
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(() => {
window.removeEventListener("resize", setScreenSize);
});
</script>

<style lang="scss">
Expand Down
Loading