Convert parameter metadata into a front-end form #23

david-mears-2 · 2024-09-03T11:25:01Z

In this PR we go as far as using the metadata response from the R API (which contains information about the model parameters) to render a form. This front-end component (ParameterForm.vue) can currently cope with two types of parameterType: select, and globeSelect, both of which result in the same kind of form input. The resulting form input is rendered depending on the number and length of the options either as a select input or a set of radio buttons (stylised to look like a row of buttons).

This form can't yet be submitted - that's for the next PR.

We could have put metadata info into a pinia AppStore or some such, but since it's only used in one page (two components) for now, I haven't bothered with it yet; but I'm bearing in mind that this might be the simpler way forward. I will probably do that in the next PR too. This AppStore could also hold the 'versions' API response.

david-mears-2 · 2024-09-03T11:25:37Z

server/utils/defineRApiEventHandler.ts

-import type { EventHandler, EventHandlerRequest } from "h3";
+import type { EventHandler, H3Event } from "h3";
 import type { ApiResponse } from "@/types/daedalusApiResponseTypes";

-export const defineEventHandlerWithErrors = <T extends EventHandlerRequest, D>(
-  handler: EventHandler<T, D>,
-): EventHandler<T, D> =>
-  defineEventHandler<T>(async (event) => {
-    const response = await handler(event) as ApiResponse;
+// A wrapper for Nuxt's defineEventHandler that handles errors from the R API.
+export const defineRApiEventHandler = (
+  callback: (event: H3Event) => Promise<ApiResponse>,
+): EventHandler =>
+  defineEventHandler(async (event) => {
+    const response = await callback(event) as ApiResponse;


These changes were motivated by this comment: #10 (comment)

codecov · 2024-09-03T11:26:30Z

Codecov Report

Attention: Patch coverage is 68.77637% with 74 lines in your changes missing coverage. Please review.

Project coverage is 58.70%. Comparing base (bee45e8) to head (29a6f0f).
Report is 22 commits behind head on main.

Files with missing lines	Patch %	Lines
modules/coreui.ts	0.00%	45 Missing ⚠️
components/ParameterIcon.vue	68.96%	9 Missing ⚠️
server/api/metadata.get.ts	0.00%	7 Missing and 1 partial ⚠️
server/utils/defineRApiEventHandler.ts	0.00%	4 Missing and 1 partial ⚠️
server/api/versions.get.ts	0.00%	4 Missing ⚠️
components/ParameterForm.vue	97.27%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #23      +/-   ##
==========================================
+ Coverage   54.02%   58.70%   +4.67%     
==========================================
  Files          22       26       +4     
  Lines         509      724     +215     
  Branches       35       63      +28     
==========================================
+ Hits          275      425     +150     
- Misses        226      290      +64     
- Partials        8        9       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…arameters

This was to avoid having a handler fed into a another defineEventHandler which would wrap the first EventHandler in a second EventHandler. Although the docs pointed us in the direction of doing such a double-wrapping, we found this way to avoid doing so. A downside of this is that endpoints are forced to use a specific function (e.g. 'defineEventHandler' or in this case 'defineCachedEventHandler') since they all route through the wrapper's choice of function - this means if we wanted to use e.g. defineLazyEventHandler from h3 in some endpoint, it (probably?) couldn't use our custom wrapper, unless we introduce some switch statement that lets endpoints specify which EventHandler wrapping function to use. I am aware of the following wrapping function choices: - defineEventHandler - defineCachedEventHandler - defineLazyEventHandler

Cacheing was too complicated - there was no easy way to purge the cache for testing different responses from the same endpoint within a test suite.

EmmaLRussell

Looks great! Couple of minor comments. That coreui eccentricity is a bit concerning...

EmmaLRussell · 2024-09-04T15:16:09Z

components/ParameterForm.vue

+    case "vaccine":
+      return "cilIndustry";
+    case "pathogen":
+      return "cilBug";


The bug is cute but I wonder if it's going to make people immersed in disease response think about vector-borne illness particularly. And maybe cute isn't really appropriate anyway!.. Not sure - see what other people think!

Also - arguably the preferred icon should be provided as part of the metadata too.. It is a presentation detail, but there could be a layer of abstraction from API icon type to front end icon type.

arguably the preferred icon should be provided as part of the metadata too

I'm not sure I see the benefit. If there is an abstraction layer, we'd still end up with the web app having a dict look up.

You make a good point about the bug icon, I wasn't sure myself. I will look for a little virus icon that matches our style.

The colour's a little off, making my eye twitch, but I hope people can live with it for now because I couldn't quickly fix it (when you have inline svg you can change a fill attribute, but that doesn't seem to work when it's an svg loaded in an img tag)

EmmaLRussell · 2024-09-04T15:26:05Z

components/ParameterForm.vue

+        <CFormSelect
+          :id="globeParameter.id"
+          v-model="formData[globeParameter.id]"
+          :data-test="`${globeParameter.id}-select`"
+          :label="globeParameter.label"
+          :aria-label="globeParameter.label"
+          :options="selectOptions(globeParameter)"
+          :size="largeScreen ? 'lg' : ''"


Because there's a bit of repetition here with the select parameter types, I wonder if it would be as well to just have a single v-for for both select and globe parameter types. That way you'll also be able to include the drop-down for the globe parameter in the position returned by the metadata, which would be more flexible I think. It doesn't necessarily feel the most natural to have the country parameter last (or first).

(And if we're going to honour the parameter order defined by the metadata it should be the same v-for for numeric parameters too eventually.)

Would mean slightly rejigging the logic for getting the icons etc.

EmmaLRussell · 2024-09-04T15:30:39Z

components/ParameterForm.vue

+
+const props = defineProps<{
+  globeParameter: Parameter | undefined
+  metaData: MetaData | undefined


Suggested change

metaData: MetaData | undefined

metadata: Metadata | undefined

🙏

EmmaLRussell · 2024-09-04T15:33:13Z

components/ParameterForm.vue

+    }
+
+    return accumulator;
+  }, {} as { [key: string]: any }),


Suggested change

}, {} as { [key: string]: any }),

}, {} as { [key: string]: string | number }),

Turns out one of the 3rd-party components we're using, the radio button one, dictates that the type signature for the 'v-model' prop -- (which it translates into 'modelValue' behind the scenes!) -- is string | boolean | unknown[] | undefined. Notice it does not allow number.

From this, I will omit the Array type because we expect not to have multi-selects here.

I think I will keep the number type around as suggested, so it can be used by numeric inputs and we can do calculations with the value; and I'll just cast all values into strings when they are passed into the radio button v-model prop.

EmmaLRussell · 2024-09-04T15:41:49Z

components/ParameterForm.vue

+}>();
+
+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 refs with default values.


Rather than these individual refs, how about using reactive

This seems to work:

const formData = ref( props.metadata?.parameters.reduce((acc, { id, defaultOption, options }) => { acc[id] = defaultOption || options[0].id; return acc; }, {} as { [key: string]: string | number }), );

And swapping out ref/reactive still passes all tests. So we can avoid having individual refs as I had before while still wrapping the whole thing in a ref() instead of a reactive(). Personally, I prefer the explicitness of having to type .value, which reminds me when I'm using something that's reactive.

EmmaLRussell · 2024-09-04T16:35:38Z

components/ParameterForm.vue

+  const eachOptionIsASingleWord = parameter.options.filter((option) => {
+    return option.label.includes(" ");
+  }).length === 0;


Suggested change

const eachOptionIsASingleWord = parameter.options.filter((option) => {

return option.label.includes(" ");

}).length === 0;

const eachOptionIsASingleWord = !parameter.options.some((option) => {

return option.label.includes(" ");

});

EmmaLRussell · 2024-09-04T16:38:21Z

components/ParameterForm.vue

+const largeScreen = ref(true);
+const breakpoint = 992; // CoreUI's "lg" breakpoint
+const setFieldSizes = () => {
+  if (window.innerWidth < breakpoint) {
+    largeScreen.value = false;
+  } else {
+    largeScreen.value = true;
+  }
+};
+


This is repeated in SideBar - should really only be implemented in one place and ideally called in one place and then made available to all components. Could be put in the store and made reactive (in another ticket!)

I'll add this to the app-store plan!

EmmaLRussell · 2024-09-04T16:39:47Z

docker/Dockerfile

@@ -7,6 +7,7 @@ ENV NODE_ENV=production
 WORKDIR /src

 COPY . .
+RUN npm install -g npm@latest


Did out of date npm cause problems in this ticket? Could update the base image if you want to use a more recent npm. Oh, is this related to rollup issue you describe below? Either way could maybe put a comment here because I'm a still a bit confused!

I don't know if it was related, but it was something I did while trying to debug the rollup issue. While working on that, I updated my local npm version, which then put my machine 1 minor version ahead of the one used in the Docker container. It seemed like a good idea to use the latest npm available in both places?

EmmaLRussell · 2024-09-04T16:45:44Z

package.json

+    "3": "the installation section of https://pinia.vuejs.org/ssr/nuxt.html",
+    "4": "==============================",
+    "5": "@rollup/rollup-linux-x64-gnu is an optional dependency as a fix for the issue that Rollup",
+    "6": "describes here: https://github.com/rollup/rollup/blob/f83b3151e93253a45f5b8ccb9ccb2e04214bc490/native.js#L59",
+    "7": "and which occurred for us when doing an installation with npm on Docker on CI. Their suggested fix",
+    "8": "does not work for our use case, because removing package-lock.json prevents the use of `npm ci`, so instead",
+    "9": "we use the solution suggested here: https://github.com/vitejs/vite/discussions/15532#discussioncomment-10192839"


Tbh, I find this trick for putting comments into package.json pretty unreadable. Given that the dependencies etc being referred to aren't actually in the vicinity of the relevant section of package.json anyway, I'm not sure there's much benefit in keeping them here. How about moving them to a section in the README file dedicated to npm dependency notes? That way you could make all these urls into working links!

That sounds better - they're pretty fiddly to write as well because of the line numbers!

EmmaLRussell · 2024-09-04T16:51:27Z

types/daedalusApiResponseTypes.ts

+  ordered: boolean
+  options: Array<ParameterOption>
+}
+export interface MetaData {


I don't think MetaData needs a hump in the middle... 🐫

This helps us to preserve the order of parameters in the metadata.

We found the documentation and behavior of the CFormSelect component too confusing to be worth any benefit. In particular, it did not seem to honour the initial v-model value, so we had to manually set the 'selected' attribute for each option (which results in a recalculation of options every time one is selected) in order to get the correct default value to be honoured - and even then, it was confusingly slow or flakey about it.

… enum

EmmaLRussell

looks good! Are we supposed to include an attribution for that wikimedia svg?

david-mears-2 · 2024-09-09T14:37:42Z

Are we supposed to include an attribution for that wikimedia svg?

Well caught. Yes we are, "in any reasonable manner". I shall add a note to the ticket for the acknowledgements page (that sounds like less work than adapting a new svg with a more licentious license).

https://commons.wikimedia.org/wiki/File:Coronavirus_icon.svg

EmmaLRussell · 2024-09-09T15:01:30Z

Are we supposed to include an attribution for that wikimedia svg?

Well caught. Yes we are, "in any reasonable manner". I shall add a note to the ticket for the acknowledgements page (that sounds like less work than adapting a new svg with a more licentious license).

https://commons.wikimedia.org/wiki/File:Coronavirus_icon.svg

Thanks, sounds good

david-mears-2 commented Sep 3, 2024

View reviewed changes

david-mears-2 force-pushed the jidea-57-non-country-inputs-front-end branch 5 times, most recently from 8511426 to c092e14 Compare September 3, 2024 12:10

david-mears-2 added 10 commits September 3, 2024 13:35

Initial work in progress on using metadata to render input form for p…

5b9ec4c

…arameters

Remove cacheing from r api event handler

c424b3c

Cacheing was too complicated - there was no easy way to purge the cache for testing different responses from the same endpoint within a test suite.

More updates on parameter input form

51d7190

Remove Globe component for now

fb022a6

Tidy ParameterForm.vue

8329d07

Fake some country ISOs for mock metadata

84478f2

Remove comments

bbbd6b8

lint

02bef86

Fix e2e test

83228cd

david-mears-2 force-pushed the jidea-57-non-country-inputs-front-end branch from fcf2714 to 40bcb92 Compare September 3, 2024 12:37

Fix Docker on CI issue

0ee4b83

david-mears-2 force-pushed the jidea-57-non-country-inputs-front-end branch from 40bcb92 to 0ee4b83 Compare September 3, 2024 12:49

david-mears-2 requested a review from EmmaLRussell September 3, 2024 12:50

Correct a comment

d90e865

EmmaLRussell reviewed Sep 4, 2024

View reviewed changes

david-mears-2 added 7 commits September 5, 2024 15:58

Replace icon with custom svg and refactor into ParameterIcon component

f3c693f

Use a single v-for to iterate over all parameters

cf9eefb

This helps us to preserve the order of parameters in the metadata.

Make styles consistent, and re-format one function

119419d

Move package.json comments into README.md

5bf180c

Update mocked metadata endpoint return value

4dcc7c0

Change spelling (m/M)etaData -> (m/M)etadata

19dc5db

Add type signature to ParameterForm formData and create ParameterType…

529fefc

… enum

david-mears-2 force-pushed the jidea-57-non-country-inputs-front-end branch from 6a756bb to 529fefc Compare September 5, 2024 17:03

Make ParameterForm formData terser

29a6f0f

david-mears-2 requested a review from EmmaLRussell September 5, 2024 21:28

EmmaLRussell approved these changes Sep 9, 2024

View reviewed changes

david-mears-2 merged commit 45014aa into main Sep 9, 2024
6 checks passed

david-mears-2 deleted the jidea-57-non-country-inputs-front-end branch September 9, 2024 15:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert parameter metadata into a front-end form #23

Convert parameter metadata into a front-end form #23

david-mears-2 commented Sep 3, 2024 •

edited

Loading

david-mears-2 Sep 3, 2024

codecov bot commented Sep 3, 2024 •

edited

Loading

EmmaLRussell left a comment

EmmaLRussell Sep 4, 2024

EmmaLRussell Sep 4, 2024

david-mears-2 Sep 5, 2024 •

edited

Loading

david-mears-2 Sep 5, 2024 •

edited

Loading

david-mears-2 Sep 5, 2024

david-mears-2 Sep 5, 2024

EmmaLRussell Sep 4, 2024

david-mears-2 Sep 5, 2024

EmmaLRussell Sep 4, 2024

EmmaLRussell Sep 4, 2024

david-mears-2 Sep 5, 2024 •

edited

Loading

EmmaLRussell Sep 4, 2024

david-mears-2 Sep 5, 2024 •

edited

Loading

EmmaLRussell Sep 4, 2024

EmmaLRussell Sep 4, 2024

david-mears-2 Sep 5, 2024

EmmaLRussell Sep 4, 2024

david-mears-2 Sep 5, 2024

EmmaLRussell Sep 4, 2024

david-mears-2 Sep 5, 2024

EmmaLRussell Sep 4, 2024

EmmaLRussell left a comment

david-mears-2 commented Sep 9, 2024

EmmaLRussell commented Sep 9, 2024

	metaData: MetaData \| undefined
	metadata: Metadata \| undefined

	}, {} as { [key: string]: any }),
	}, {} as { [key: string]: string \| number }),

Convert parameter metadata into a front-end form #23

Convert parameter metadata into a front-end form #23

Conversation

david-mears-2 commented Sep 3, 2024 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Sep 3, 2024 • edited Loading

Codecov Report

EmmaLRussell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david-mears-2 Sep 5, 2024 • edited Loading

Choose a reason for hiding this comment

david-mears-2 Sep 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david-mears-2 Sep 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david-mears-2 Sep 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EmmaLRussell left a comment

Choose a reason for hiding this comment

david-mears-2 commented Sep 9, 2024

EmmaLRussell commented Sep 9, 2024

david-mears-2 commented Sep 3, 2024 •

edited

Loading

codecov bot commented Sep 3, 2024 •

edited

Loading

david-mears-2 Sep 5, 2024 •

edited

Loading

david-mears-2 Sep 5, 2024 •

edited

Loading

david-mears-2 Sep 5, 2024 •

edited

Loading

david-mears-2 Sep 5, 2024 •

edited

Loading