[NM-82] download results state refactor #1036

M-Kusumgar · 2024-11-20T20:12:37Z

Description

This one was painful. Ill remove AGYW in the next PR to see how easy/hard it is to modify this new construction, almost 2000 lines gone, damn.

There was a handy enum in the types file called DownloadType so i thought way not roll with that, everything is constructed from downloadConfig.ts, using Object.values on a string enum (does not work well on a usual enum, can try it if you want to know a stupid typescript decision) gives all the values and we can loop over it to do everything

The tests were especially painful, i suspect there'll be small simplifications when we remove AGYW

The other thing is I had to add a payload handler to the api service because i needed finer control over the payload the api need to commit to the state (needed to add type so that it knows which DownloadType to commit to)

ideally, to add another button you just need to extend the 3 things in downloadConfig.ts

Edit: the e2e test failure is a real one! i am removing AGYW in #1037 which im merging into here so will just fix the e2e test there

notion-workspace · 2024-11-20T20:12:45Z

Refactor downloadResultsState and remove code duplication

r-ash

This looks really great, so good to see so much duplication removed.

Couple of comments on a few bits would be nice to get your thoughts on. But I think this looks good to go

r-ash · 2024-11-21T10:49:22Z

src/app/static/src/app/components/downloadResults/UploadModal.vue

@@ -89,6 +89,7 @@
    import {RootState} from "../../root";
    import {DownloadResultsState} from "../../store/downloadResults/downloadResults";
    import {defineComponent} from "vue";
+import { DownloadType } from "../../store/downloadResults/downloadConfig";


Bit of weird indenting

ahhh my auto import extension is trippin, fixed

r-ash · 2024-11-21T10:51:06Z

src/app/static/src/app/store/downloadResults/actions.ts

-    async downloadComparisonReport(context) {
-        const {state, commit} = context
-        commit({type: DownloadResultsMutation.ComparisonDownloadError, payload: null})
-        await api(context)
-            .ignoreSuccess()
-            .withError(DownloadResultsMutation.ComparisonDownloadError)
-            .download(`download/result/${state.comparison.downloadId}`)
-    },
-
-    async downloadSpectrumOutput(context) {
-        const {state, commit} = context
-        commit({type: DownloadResultsMutation.SpectrumOutputDownloadError, payload: null})
-        await api(context)
-            .ignoreSuccess()
-            .withError(DownloadResultsMutation.SpectrumOutputDownloadError)
-            .download(`download/result/${state.spectrum.downloadId}`)
-    },
-
-    async downloadSummaryReport(context) {
-        const {state, commit} = context
-        commit({type: DownloadResultsMutation.SummaryOutputDownloadError, payload: null})
-        await api(context)
-            .ignoreSuccess()
-            .withError(DownloadResultsMutation.SummaryOutputDownloadError)
-            .download(`download/result/${state.summary.downloadId}`)
-    },
-
-    async downloadCoarseOutput(context) {
-        const {state, commit} = context
-        commit({type: DownloadResultsMutation.CoarseOutputDownloadError, payload: null})
-        await api(context)
-            .ignoreSuccess()
-            .withError(DownloadResultsMutation.CoarseOutputDownloadError)
-            .download(`download/result/${state.coarseOutput.downloadId}`)
-    },
-
-    async downloadAgywTool(context) {
-        const {state, commit} = context
-        commit({type: DownloadResultsMutation.AgywDownloadError, payload: null})
-        await api(context)
-            .ignoreSuccess()
-            .withError(DownloadResultsMutation.AgywDownloadError)
-            .download(`download/result/${state.agyw.downloadId}`)
-    },
-
-    async prepareCoarseOutput(context) {
-        const {state, dispatch, rootState, commit} = context
-        if (!state.coarseOutput.downloadId && !state.coarseOutput.fetchingDownloadId) {
-            commit({type: "SetFetchingDownloadId", payload: DOWNLOAD_TYPE.COARSE});
-            const calibrateId = rootState.modelCalibrate.calibrateId
-            const response = await api<DownloadResultsMutation, DownloadResultsMutation>(context)
-                .withSuccess(DownloadResultsMutation.PreparingCoarseOutput)
-                .withError(DownloadResultsMutation.CoarseOutputError)
-                .postAndReturn(`download/submit/coarse-output/${calibrateId}`, {})
-
-            if (response) {
-                await dispatch("poll", DOWNLOAD_TYPE.COARSE)
-            }
-        }
-    },
-
-    async prepareSummaryReport(context) {
-        const {state, dispatch, rootState, commit} = context
-        if (!state.summary.downloadId && !state.summary.fetchingDownloadId) {
-            commit({type: "SetFetchingDownloadId", payload: DOWNLOAD_TYPE.SUMMARY});
-            const calibrateId = rootState.modelCalibrate.calibrateId
-            const response = await api<DownloadResultsMutation, DownloadResultsMutation>(context)
-                .withSuccess(DownloadResultsMutation.PreparingSummaryReport)
-                .withError(DownloadResultsMutation.SummaryError)
-                .postAndReturn(`download/submit/summary/${calibrateId}`, {})
-
-            if (response) {
-                await dispatch("poll", DOWNLOAD_TYPE.SUMMARY)
-            }
-        }
-    },


Love to see it

r-ash · 2024-11-21T10:51:43Z

src/app/static/src/app/store/downloadResults/actions.ts

@@ -3,273 +3,83 @@ import {RootState} from "../../root";
 import {DownloadResultsState} from "./downloadResults";
 import {api, ResponseWithType} from "../../apiService";
 import {DownloadResultsMutation} from "./mutations";
-import {ModelStatusResponse} from "../../generated";
-import {DOWNLOAD_TYPE} from "../../types";
+import {Error, ModelStatusResponse} from "../../generated";


Error not used 🤓

yep removed, neither was switches in the next line! you can tell i did a couple of drafts before settling on the final version haha

r-ash · 2024-11-21T10:54:28Z

src/app/static/src/app/store/downloadResults/downloadConfig.ts

+    [DownloadType.SPECTRUM]: { url: "spectrum", body: (store) => store.rootGetters.projectState },
+    [DownloadType.COARSE]: { url: "coarse-output", body: () => ({}) },
+    [DownloadType.SUMMARY]: { url: "summary", body: () => ({}) },
+    [DownloadType.COMPARISON]: { url: "comparison", body: () => ({}) },
+    [DownloadType.AGYW]: { url: "agyw", body: () => ({}) }


Small point but I think I would favour adding the full URL (minus the ID) here instead of some here and some in prepareOutput. Makes it a little easier to find if in one place

agreed, in fact lets do a bit more generally, lets just make both the body and url functions that take in the store, that would be good!

r-ash · 2024-11-21T10:57:26Z

src/app/static/src/app/store/downloadResults/mutations.ts

-    AgywError = "AgywError",
-    AgywStatusUpdated = "AgywStatusUpdated",
-    AgywDownloadError = "AgywDownloadError",
+    Preparing = "Preparing",


So good, fewer than half the original number

yh these were definitely satisfying files!

r-ash · 2024-11-21T11:01:48Z

src/app/static/src/app/components/downloadResults/DownloadResults.vue

-                    <error-alert v-if="spectrum.downloadError" :error="spectrum.downloadError"></error-alert>
+                <template v-for="type in Object.values(DownloadType)" :key="type">
+                    <div :id="`${type}-download`" v-if="switches[type]">
+                        <download :translate-key="`${type}Download`"


I wonder if we should put this into the downloadConfig.ts like the switches. A map of type to translation key. I do like all the download stuff being in the single config file.

hmmm do you mean because theres duplication between this and the locales.ts type? because otherwise everything is auto generated, also the types in the locales file ensures we have the matching translation key

No, only that when you're adding a new file type you have to look here to see what the key is. It is {type}Download" instead of just being able to see the full string in the downloadConfig.ts` ? That make sense?

done extracted

r-ash · 2024-11-21T11:03:06Z

src/app/static/src/app/store/adrUpload/actions.ts

@@ -111,23 +112,23 @@ export const actions: ActionTree<ADRUploadState, RootState> & ADRUploadActions =
                requestParams["resourceId"] = resourceId
            }
            if (resourceType === rootState.adr.schemas?.outputSummary) {
-                downloadId = rootState.downloadResults.summary.downloadId
+                downloadId = rootState.downloadResults[DownloadType.SUMMARY].downloadId


What do you also think about handling this through the downloadConfig? Have some list of files which need to go to the ADR, and here look through the DownloadType and include those which are in the list?

Could use it above in UploadModal

the reason i didnt do this here is because this requires me to refactor the ADR upload stuff because we need some way of matching up if "outputSummary" exists, then include DownloadType.SUMMARY, i say we do a smaller refactor in a separate PR and sort out the adrUpload stuff and there we can put these mappings, infact some of the adrUpload stuff can probably just come from the downloadConfig itself

in the UploadModal we have this same mapping issue and also have outputFileError which has a weird control flow which does something different if both spectrum and summary have metadata errors vs just one of them, i would like to properly investigate that and see if theres a pattern that we can extract

i can create a ticket for these two if you agree!

Yep sounds great doing that separately, yes please create the tickets.

r-ash · 2024-11-21T11:04:55Z

src/app/static/src/app/store/root/mutations.ts

@@ -141,6 +136,12 @@ export const mutations: MutationTree<RootState> = {

 };

+const stopPollingAllDownloadTypes = (state: RootState) => {


r-ash · 2024-11-21T11:13:53Z

src/app/static/src/app/apiService.ts

+    withError = (type: E, root = false, payloadFunc?: (e: Error) => any) => {
        this._onError = (failure: Response) => {
-            this._commit({type: type, payload: APIService.getFirstErrorFromFailure(failure)}, {root});
+            const error = APIService.getFirstErrorFromFailure(failure);
+            this._commit({ type, payload: payloadFunc ? payloadFunc(error) : error }, { root });


So this is what I had previously added withErrorCallback for. But I think what you've done is better. Could you remove the withErrorCallback helper and tidy up uses of that to use this instead?

But actually, I wonder if we should have 2 functions withError and withErrorHandler? Or some better name? Whatever you think!

ohhhh damn sorry i didnt even see that, i was looking for that type of functionality, yh im happy to remove it, i think its nice to declare the mutation you want and all that, i personally think its nicer if we just have one function with an optional arg, it feels like exactly the same responsibility, just with pre commit step or not

im basically not sure what we gain by having 2 separate functions XD

Nope that's fine, happy with 1 function if you want.

M-Kusumgar · 2024-11-21T19:10:51Z

have addressed comments in this commit: 481b209

ADR upload refactor ticket is created!: https://www.notion.so/unaids-naomi/Upload-ADR-state-refactor-f1c71f2f341c49aa98bb9f3bad88e99c?pvs=4

r-ash

Thanks for that, this is looking great. Just merged main into it and updated the download test

r-ash

Thanks for that, I just merged in #1034 and then removed downloadOutput action. We don't need that any more as we're now downloading via an href instead of an action

refactor done

493c0ab

clean up downloadConfig

60bd6c5

M-Kusumgar requested a review from r-ash November 20, 2024 20:25

Merge branch 'main' of github.com:hivtools/hint into nm-82

849b876

r-ash mentioned this pull request Nov 21, 2024

[NM-9] Replace file download with native browser download #1034

Merged

2 tasks

r-ash reviewed Nov 21, 2024

View reviewed changes

r-ash mentioned this pull request Nov 21, 2024

[NM-84] Remove AGYW #1037

Merged

Merge branch 'main' into nm-82

c1a6b7c

r-ash self-requested a review November 21, 2024 20:50

r-ash approved these changes Nov 21, 2024

View reviewed changes

Remove downloadOutput action

d54e89e

r-ash approved these changes Nov 21, 2024

View reviewed changes

r-ash merged commit fa30f27 into main Nov 21, 2024
7 of 9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NM-82] download results state refactor #1036

[NM-82] download results state refactor #1036

M-Kusumgar commented Nov 20, 2024 •

edited

Loading

notion-workspace bot commented Nov 20, 2024

r-ash left a comment

r-ash Nov 21, 2024

M-Kusumgar Nov 21, 2024

r-ash Nov 21, 2024

r-ash Nov 21, 2024

M-Kusumgar Nov 21, 2024 •

edited

Loading

r-ash Nov 21, 2024

M-Kusumgar Nov 21, 2024

M-Kusumgar Nov 21, 2024

r-ash Nov 21, 2024

M-Kusumgar Nov 21, 2024

r-ash Nov 21, 2024 •

edited

Loading

M-Kusumgar Nov 21, 2024

r-ash Nov 21, 2024

M-Kusumgar Nov 21, 2024

r-ash Nov 21, 2024

r-ash Nov 21, 2024

M-Kusumgar Nov 21, 2024

r-ash Nov 21, 2024

r-ash Nov 21, 2024

r-ash Nov 21, 2024

M-Kusumgar Nov 21, 2024 •

edited

Loading

r-ash Nov 21, 2024

M-Kusumgar commented Nov 21, 2024

r-ash left a comment

r-ash left a comment

		@@ -141,6 +136,12 @@ export const mutations: MutationTree<RootState> = {

		};

		const stopPollingAllDownloadTypes = (state: RootState) => {

[NM-82] download results state refactor #1036

[NM-82] download results state refactor #1036

Conversation

M-Kusumgar commented Nov 20, 2024 • edited Loading

Description

notion-workspace bot commented Nov 20, 2024

r-ash left a 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

M-Kusumgar Nov 21, 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

r-ash Nov 21, 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

Choose a reason for hiding this comment

M-Kusumgar Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

M-Kusumgar commented Nov 21, 2024

r-ash left a comment

Choose a reason for hiding this comment

r-ash left a comment

Choose a reason for hiding this comment

M-Kusumgar commented Nov 20, 2024 •

edited

Loading

M-Kusumgar Nov 21, 2024 •

edited

Loading

r-ash Nov 21, 2024 •

edited

Loading

M-Kusumgar Nov 21, 2024 •

edited

Loading