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.

Sign up for GitHub

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

[NM-82] download results state refactor #1036

Merged
merged 5 commits into from
Nov 21, 2024
Merged

[NM-82] download results state refactor #1036

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
493c0ab
refactor done
M-Kusumgar Nov 20, 2024
60bd6c5
clean up downloadConfig
M-Kusumgar Nov 20, 2024
849b876
Merge branch 'main' of github.com:hivtools/hint into nm-82
M-Kusumgar Nov 20, 2024
c1a6b7c
Merge branch 'main' into nm-82
r-ash Nov 21, 2024
d54e89e
Remove downloadOutput action
r-ash Nov 21, 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
15 changes: 5 additions & 10 deletions src/app/static/src/app/apiService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,9 +70,10 @@ export class APIService<S extends string, E extends string> implements API<S, E>
return this;
};

withError = (type: E, root = false) => {
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 });
Comment on lines +73 to +76
Copy link
Collaborator

Choose a reason for hiding this comment

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

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!

Copy link
Contributor Author

@M-Kusumgar M-Kusumgar Nov 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

};
return this;
};
Expand All @@ -92,15 +93,9 @@ export class APIService<S extends string, E extends string> implements API<S, E>
return this;
};

withSuccess = (type: S, root = false) => {
withSuccess = (type: S, root = false, payloadFunc?: (data: any) => any) => {
this._onSuccess = (data: any) => {
const toCommit = {type: type, payload: data};
if (root) {
this._commit(toCommit, {root: true});
} else {
this._commit(toCommit);
}

this._commit({ type, payload: payloadFunc ? payloadFunc(data) : data }, { root });
};
return this;
};
Expand Down
6 changes: 3 additions & 3 deletions src/app/static/src/app/components/downloadResults/Download.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
<template>
<div id="download">
<h4 v-translate="translateKey.header"></h4>
<h4 v-translate="translateKey"></h4>
<button class="btn btn-lg my-3" :class="disabled ? 'btn-secondary' : 'btn-red'"
:disabled="disabled"
@click="download">
<span v-translate="translateKey.button"></span>
<span v-translate="'download'"></span>
<vue-feather type="download" size="20" class="icon ml-2" style="margin-top: -4px;"></vue-feather>
</button>
<div>
Expand Down Expand Up @@ -38,7 +38,7 @@
},
translateKey: {
required: true,
type: Object
type: String
},
disabled: {
required: true,
Expand Down
79 changes: 20 additions & 59 deletions src/app/static/src/app/components/downloadResults/DownloadResults.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,42 +2,16 @@
<div class="container">
<div class="row">
<div class="col-sm">
<div id="spectrum-download">
<download :translate-key="translation.spectrum"
:disabled="!spectrum.downloadId || spectrum.preparing || !!spectrum.metadataError"
:file="spectrum"/>
<div class="pb-2">
<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`"
Copy link
Collaborator

@r-ash r-ash Nov 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done extracted

:disabled="!state[type].downloadId || state[type].preparing || !!state[type].metadataError"
:file="state[type]"/>
<div class="pb-2">
<error-alert v-if="state[type].downloadError" :error="state[type].downloadError"></error-alert>
</div>
</div>
</div>
<div id="coarse-output-download">
<download :translate-key="translation.coarse"
:disabled="!coarseOutput.downloadId || coarseOutput.preparing || !!coarseOutput.metadataError"
:file="coarseOutput"/>
<div class="pb-2">
<error-alert v-if="coarseOutput.downloadError" :error="coarseOutput.downloadError"></error-alert>
</div>
</div>
<div id="summary-download">
<download :translate-key="translation.summary"
:disabled="!summary.downloadId || summary.preparing || !!summary.metadataError"
:file="summary"/>
<div class="pb-2">
<error-alert v-if="summary.downloadError" :error="summary.downloadError"></error-alert>
</div>
</div>
<div id="comparison-download" v-if="comparisonSwitch">
<download :translate-key="translation.comparison"
:disabled="!comparison.downloadId || comparison.preparing || !!comparison.metadataError"
:file="comparison"/>
<error-alert v-if="comparison.downloadError" :error="comparison.downloadError"></error-alert>
</div>
<div id="agyw-download" v-if="agywSwitch">
<download :translate-key="translation.agyw"
:disabled="!agyw.downloadId || agyw.preparing || !!agyw.metadataError"
:file="agyw"/>
<error-alert v-if="agyw.downloadError" :error="agyw.downloadError"></error-alert>
</div>
</template>
</div>
<div id="upload" v-if="hasUploadPermission" class="col-sm">
<h4 v-translate="'uploadFileToAdr'"></h4>
Expand Down Expand Up @@ -91,34 +65,31 @@
import ErrorAlert from "../ErrorAlert.vue";
import i18next from "i18next";
import {ADRUploadState} from "../../store/adrUpload/adrUpload";
import {DownloadResultsState} from "../../store/downloadResults/downloadResults";
import Download from "./Download.vue";
import {switches} from "../../featureSwitches";
import { defineComponent } from "vue";
import { downloadSwitches, DownloadType } from "../../store/downloadResults/downloadConfig";
interface Data {
uploadModalOpen: boolean,
comparisonSwitch: boolean,
agywSwitch: boolean,
DownloadType: typeof DownloadType,
switches: {
[K in DownloadType]: boolean
}
}
export default defineComponent({
name: "downloadResults",
data(): Data {
return {
uploadModalOpen: false,
comparisonSwitch: switches.comparisonOutput,
agywSwitch: switches.agywDownload,
switches: downloadSwitches,
DownloadType
}
},
computed: {
hasUploadPermission: mapStateProp<ADRState, boolean>("adr", (state: ADRState) => state.userCanUpload),
...mapStateProps("downloadResults", {
spectrum: ((state: DownloadResultsState) => state.spectrum),
summary: ((state: DownloadResultsState) => state.summary),
coarseOutput: ((state: DownloadResultsState) => state.coarseOutput),
comparison: ((state: DownloadResultsState) => state.comparison),
agyw: ((state: DownloadResultsState) => state.agyw),
state: (state) => state
}),
...mapStateProps("adrUpload", {
uploading: ((state: ADRUploadState) => state.uploading),
Expand All @@ -140,18 +111,8 @@
null,
(state: RootState) => state.language
),
translation(): Record<string, any> {
return {
spectrum: {header: 'exportOutputs', button: 'download'},
coarse: {header: 'downloadCoarseOutput', button: 'download'},
summary: {header: 'downloadSummaryReport', button: 'download'},
comparison: {header: 'downloadComparisonReport', button: 'download'},
agyw: {header: 'downloadAgywTool', button: 'download'},
}
},
isPreparing(): boolean {
return this.summary.preparing || this.spectrum.preparing || this.coarseOutput.preparing ||
this.comparison.preparing || this.agyw.preparing
return Object.values(DownloadType).some(type => this.state[type].preparing);
}
},
methods: {
Expand All @@ -161,12 +122,12 @@
clearStatus: mapMutationByName("adrUpload", "ClearStatus"),
getUserCanUpload: mapActionByName("adr", "getUserCanUpload"),
getUploadFiles: mapActionByName("adrUpload", "getUploadFiles"),
prepareOutputs: mapActionByName("downloadResults", "prepareOutputs"),
prepareAllOutputs: mapActionByName("downloadResults", "prepareAllOutputs")
},
mounted() {
this.getUserCanUpload();
this.getUploadFiles();
this.prepareOutputs();
this.prepareAllOutputs();
},
beforeMount() {
this.clearStatus();
Expand Down
9 changes: 4 additions & 5 deletions src/app/static/src/app/components/downloadResults/UploadModal.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@
import {RootState} from "../../root";
import {DownloadResultsState} from "../../store/downloadResults/downloadResults";
import {defineComponent} from "vue";
import { DownloadType } from "../../store/downloadResults/downloadConfig";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Bit of weird indenting

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ahhh my auto import extension is trippin, fixed

interface Data {
uploadFilesToAdr: string[]
Expand Down Expand Up @@ -158,18 +159,16 @@
},
computed: {
...mapStateProps("downloadResults", {
summary: ((state: DownloadResultsState) => state.summary),
spectrum: ((state: DownloadResultsState) => state.spectrum),
comparison: ((state: DownloadResultsState) => state.comparison)
summary: (state: DownloadResultsState) => state[DownloadType.SUMMARY],
spectrum: (state: DownloadResultsState) => state[DownloadType.SPECTRUM],
comparison: (state: DownloadResultsState) => state[DownloadType.COMPARISON]
}),
outputFileError(): string | null {
if ((this.summary.error || this.summary.metadataError) &&
(this.spectrum.error || this.spectrum.metadataError)) {
return "downloadSpectrumAndSummaryError"
} else if (this.summary.error || this.summary.metadataError) {
return this.translatedOutputFileError("downloadSummary")
} else if (this.spectrum.error || this.spectrum.metadataError) {
return this.translatedOutputFileError("downloadSpectrum")
} else if (this.comparison.error || this.comparison.metadataError) {
Expand Down
7 changes: 4 additions & 3 deletions src/app/static/src/app/store/adrUpload/actions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import {ADRUploadMutation} from "./mutations";
import {constructUploadFile, constructUploadFileWithResourceName} from "../../utils";
import {Dict, UploadFile} from "../../types";
import {ValidateInputResponse} from "../../generated";
import { DownloadType } from "../downloadResults/downloadConfig";

export interface ADRUploadActions {
getUploadFiles: (store: ActionContext<ADRUploadState, RootState>) => void;
Expand Down Expand Up @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could use it above in UploadModal

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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!

Copy link
Collaborator

Choose a reason for hiding this comment

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

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


requestParams["description"] =
uploadMetadata?.find(meta => meta.type === "summary")?.description
|| "Naomi summary report uploaded from Naomi web app"
}

if (resourceType === rootState.adr.schemas?.outputZip) {
downloadId = rootState.downloadResults.spectrum.downloadId
downloadId = rootState.downloadResults[DownloadType.SPECTRUM].downloadId

requestParams["description"] =
uploadMetadata?.find(meta => meta.type === "spectrum")?.description
|| "Naomi output uploaded from Naomi web app"
}

if (resourceType === rootState.adr.schemas?.outputComparison) {
downloadId = rootState.downloadResults.comparison.downloadId
downloadId = rootState.downloadResults[DownloadType.COMPARISON].downloadId

requestParams["description"] =
uploadMetadata?.find(meta => meta.type === "comparison")?.description
Expand Down
Loading
Loading