Bacpop-163 Error handle + delete sample #60

absternator · 2024-03-18T13:21:10Z

This PR contains error handling with the projectStore and also functionality to delete sample files. Elobarted more in detail below:

Basic error handling in project page. This is mostly toast messages in the projectStore.ts
Abiloty to remove file samples from un run projects. Added an endpoint in API and also UI to accompany this.
Unit testing

For testing its best to test the removal of file samples when project is not run. Refresh and ensure that it is persisted.

… zip file

into bacpop-163-error-handling

codecov · 2024-03-18T13:36:54Z

Codecov Report

Attention: Patch coverage is 94.44444% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 98.61%. Comparing base (98174eb) to head (c907cdd).
Report is 1 commits behind head on bacpop-148-ui-rewrite.

Files	Patch %	Lines
app/client-v2/src/stores/projectStore.ts	89.28%	3 Missing ⚠️

Additional details and impacted files

@@                    Coverage Diff                    @@
##           bacpop-148-ui-rewrite      #60      +/-   ##
=========================================================
- Coverage                  98.80%   98.61%   -0.20%     
=========================================================
  Files                         23       24       +1     
  Lines                       1337     1367      +30     
  Branches                     157      162       +5     
=========================================================
+ Hits                        1321     1348      +27     
- Misses                        16       19       +3

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

into bacpop-163-error-handling

EmmaLRussell

Branches are easier to review when they focus on a single bit of functionality so I think it would be have been nicer to keep these two things separate. Or explain why they need to go together. It sometimes seems easier to roll stuff together, but you're also potentially holding up unrelated functionality from being merged if the other bit needs changing...

I don't think toast is sufficient for error reporting. It's too ephemeral, The user needs to see and potentially report to us what the error was, and toast is too easy to miss. I know you're logging to the console, but most users aren't going to be looking there.

Also, by separation of concerns, the store should not have any knowledge of how errors are displayed by the UI, so showErrorToast should move out of projectStore (and not make any reference to toast!). Really errors should become store values and the components should display those.

Could potentially make errors some dictionary in the store, and could have an Error component which takes a dictionary key as a prop (e.g. deleteUploadedFileError) it would display that error from the store if present, and also give user to option to dismiss the error.

(Or maybe could stick with toast if you make it effectively live long enough that it's permanent until the user dismisses it. But then you have the danger of toast proliferation, and that's not really how toast is supposed to be used...)

I notice that you're not including any raw error details in the errors shown on screen. I guess the idea is that these are probably too gross to show a user, but I think it can be really useful for us to debug if user can report those to us, without us having to go round the loop of getting them to run it up again and check the console.. Could potentially go under a 'Details' link as a compromise.

(Unrelated rant: Not your fault, but I really don't like having tests in the __tests__ folder as a reviewer, as it means when you're scrolling through changes you hit the tests before you see (and understand) the src that they're testing. I know it's the standard scaffolding, but I'm not a fan! 😆 I suppose we could change it but it's probably not worth it. I guess the TDD mob would say it's better to see the tests first anyway... What do you think?)

EmmaLRussell · 2024-03-20T09:23:56Z

app/client-v2/src/__tests__/components/ProjectView/ProjectPage.spec.ts

+vitest.mock("primevue/usetoast", () => ({
+  useToast: vitest.fn()
+}));


It won't save that many lines of code, but since this crops up loads of times, could make a little mockToast util. However, see above - I don't actually think toast is the way to go.

i just tried this and spent time getting it to work but has issues.. so I have left as is

EmmaLRussell · 2024-03-20T09:37:39Z

app/client-v2/src/stores/projectStore.ts

@@ -83,6 +93,10 @@ export const useProjectStore = defineStore("project", {
        worker.onmessage = async (event: MessageEvent<WorkerResponse>) => {
          this.handleWorkerResponse(file.name, event);
        };
+        worker.onerror = (error) => {
+          console.error(error);
+          this.showErrorToast("Ensure uploaded sample file is correct or try again later.");


Trying again later isn't going to make any difference if it's a front end processing error rather than an upload error. I feel like it should be a different message for these two scenarios.

have removed the try again for this one

EmmaLRussell · 2024-03-20T09:42:58Z

app/client-v2/src/stores/projectStore.ts

@@ -136,6 +153,7 @@ export const useProjectStore = defineStore("project", {
          stopPolling = true;
        }
      } catch (error) {
+        this.showErrorToast("Error fetching analysis status. Try again later, or create a new project.");


Not sure what "Try again later" would mean in this context, as the user doesn't invoke status fetch themselves.

i thought if issues with server down or something.. but have changed to try refresh pag instead

EmmaLRussell · 2024-03-20T09:45:11Z

app/client-v2/src/stores/projectStore.ts

+        this.project.status = { assign: "submitted", microreact: "submitted", network: "submitted" };
+        this.pollAnalysisStatus();


Why is this new code? Has it moved from somewhere else?

nope just shifted up a bit into try catch block

EmmaLRussell · 2024-03-20T09:49:17Z

app/server/src/routes/projectRoutes.ts

+        app.post(`/project/:projectId/sample/:sampleHash/delete`,
+            authCheck, 
+            controller.deleteSample);


Should be a DELETE?

initially i thought that too but we are deleting part of resource and also as comment below changed to patch and can pass in filename in body

EmmaLRussell · 2024-03-20T09:50:13Z

app/client-v2/src/components/ProjectView/ProjectPreRun.vue

@@ -15,7 +15,7 @@ const tableIsHovered = ref(false);
        <Button label="Run Analysis" outlined @click="store.runAnalysis" :disabled="!store.isReadyToRun" />
      </div>
    </template>
-    <template #content>
+    <template #empty>


Why this change?

realised it shows error message if file uploaded is wrong (wrong extension, too big etc)

EmmaLRussell · 2024-03-20T10:07:19Z

app/server/src/db/userStore.ts

@@ -98,6 +98,12 @@ export class UserStore {
        const amrString = await this._redis.hget(this._projectSampleKey(projectId, sampleId), "amr");
        return JSON.parse(amrString);
    }
+    async deleteSample(projectId: string, sampleHash: string) {
+        const sampleIds = await this._redis.smembers(this._projectSamplesKey(projectId));
+        const sampleId = sampleIds?.find((id) => id.startsWith(sampleHash));


I don't think this quite works. In theory someone might have uploaded the same sample data twice with two different filenames (could happen!) - if they select the second one to delete, it shouldn't delete the first one, which I think is what would happen here. So I think the delete endpoint is going to have to take the filename as well as the sample hash unfortunately.

reeeeally weird why someone would do that but updated as requested

absternator · 2024-03-20T11:37:51Z

Branches are easier to review when they focus on a single bit of functionality so I think it would be have been nicer to keep these two things separate. Or explain why they need to go together. It sometimes seems easier to roll stuff together, but you're also potentially holding up unrelated functionality from being merged if the other bit needs changing...

I don't think toast is sufficient for error reporting. It's too ephemeral, The user needs to see and potentially report to us what the error was, and toast is too easy to miss. I know you're logging to the console, but most users aren't going to be looking there.

Also, by separation of concerns, the store should not have any knowledge of how errors are displayed by the UI, so showErrorToast should move out of projectStore (and not make any reference to toast!). Really errors should become store values and the components should display those.

Could potentially make errors some dictionary in the store, and could have an Error component which takes a dictionary key as a prop (e.g. deleteUploadedFileError) it would display that error from the store if present, and also give user to option to dismiss the error.

(Or maybe could stick with toast if you make it effectively live long enough that it's permanent until the user dismisses it. But then you have the danger of toast proliferation, and that's not really how toast is supposed to be used...)

I notice that you're not including any raw error details in the errors shown on screen. I guess the idea is that these are probably too gross to show a user, but I think it can be really useful for us to debug if user can report those to us, without us having to go round the loop of getting them to run it up again and check the console.. Could potentially go under a 'Details' link as a compromise.

(Unrelated rant: Not your fault, but I really don't like having tests in the __tests__ folder as a reviewer, as it means when you're scrolling through changes you hit the tests before you see (and understand) the src that they're testing. I know it's the standard scaffolding, but I'm not a fan! 😆 I suppose we could change it but it's probably not worth it. I guess the TDD mob would say it's better to see the tests first anyway... What do you think?)

I have moved toast to comparable. I have also increased toast TTL to 5seconds so sufficient for user
I agree about minimal error and toast but the problem Is we don't have sufficient error messages and most are not useful to the user. Also this is just simple error handling as part of New U, I will create a ticket for updating error states and a more robust one that will include both server/client errors.
But other errors could ephemeral (random server error or internet issue).... Also toast makes sense as most of them will occur for a long running process...
I think its only necessary to surface the basics.. And yes if they do need help we will look their client error and server logs.
tests for reviewing you can collapse the file if you want haha.. But I'm easy with the layout i don't think it matters much

…ebop into bacpop-163-error-handling

EmmaLRussell · 2024-03-25T10:28:13Z

app/client-v2/src/stores/projectStore.ts

@@ -136,6 +144,7 @@ export const useProjectStore = defineStore("project", {
          stopPolling = true;
        }
      } catch (error) {
+        this.toast.showErrorToast("Error fetching analysis status. Try refresh page, or create a new project.");


Suggested change

this.toast.showErrorToast("Error fetching analysis status. Try refresh page, or create a new project.");

this.toast.showErrorToast("Error fetching analysis status. Try refreshing the page, or create a new project.");

EmmaLRussell

Looks good. Tiny suggeston, up to you if you want to do this.

EmmaLRussell · 2024-03-25T10:32:24Z

app/server/src/routes/projectRoutes.ts

@@ -24,7 +24,7 @@ export default {
        app.post('/project/:projectId/rename',
            authCheck,
            controller.renameProject);
-        app.post(`/project/:projectId/sample/:sampleHash/delete`,
+        app.patch(`/project/:projectId/sample/:sampleHash`,


I wonder if we should still have the delete route, since the fact that it's going to delete a hash/filename combo isn't obvious from the fact that it's a patch..?

couldnt add body (need to pass file name). left as PATCH but added /delete back in to end

… run

absternator added 7 commits March 18, 2024 07:34

feat: pre run error handling

f022e89

Add error toast messages for fetching analysis status and downloading…

f622348

… zip file

feat: allow files to be removed

192c033

Add PrimeVue ToastService and Toast component

813c123

tests: fix tests by adding mock for usetoast

5687f28

Refactor ProjectPreRun component and projectStore.spec.js

b94fa7b

tests: tests delete sample endpoint

df6e1e9

absternator requested a review from EmmaLRussell March 18, 2024 13:21

Merge branch 'bacpop-161-microreact' of https://github.com/bacpop/beebop

5f3bd0d

into bacpop-163-error-handling

absternator added 4 commits March 19, 2024 08:20

Merge branch 'bacpop-161-microreact' of https://github.com/bacpop/beebop

3a58d86

into bacpop-163-error-handling

Update network tab condition in ProjectPostRun.vue

0699793

Merge branch 'bacpop-161-microreact' of https://github.com/bacpop/beebop

385f344

into bacpop-163-error-handling

Merge branch 'bacpop-161-microreact' of https://github.com/bacpop/beebop

ab9e0f3

into bacpop-163-error-handling

EmmaLRussell reviewed Mar 20, 2024

View reviewed changes

refactor: make compasable toast service

50f0cd6

Base automatically changed from bacpop-161-microreact to bacpop-160-network-tab March 20, 2024 11:44

absternator added 2 commits March 20, 2024 11:45

Merge branch 'bacpop-160-network-tab' of https://github.com/bacpop/be…

981976d

…ebop into bacpop-163-error-handling

feat: pr updates

777a4db

absternator requested a review from EmmaLRussell March 20, 2024 13:47

Merge branch 'bacpop-160-network-tab' of https://github.com/bacpop/be…

c907cdd

…ebop into bacpop-163-error-handling

Base automatically changed from bacpop-160-network-tab to bacpop-148-ui-rewrite March 20, 2024 14:57

EmmaLRussell reviewed Mar 25, 2024

View reviewed changes

EmmaLRussell approved these changes Mar 25, 2024

View reviewed changes

Fix projectStore error message and enable remove button when ready to…

6069662

… run

absternator merged commit 3b516b7 into bacpop-148-ui-rewrite Mar 25, 2024
5 of 6 checks passed

absternator deleted the bacpop-163-error-handling branch March 25, 2024 13:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bacpop-163 Error handle + delete sample #60

Bacpop-163 Error handle + delete sample #60

absternator commented Mar 18, 2024

codecov bot commented Mar 18, 2024 •

edited

Loading

EmmaLRussell left a comment

EmmaLRussell Mar 20, 2024

absternator Mar 20, 2024

EmmaLRussell Mar 20, 2024

absternator Mar 20, 2024

EmmaLRussell Mar 20, 2024

absternator Mar 20, 2024

EmmaLRussell Mar 20, 2024

absternator Mar 20, 2024

EmmaLRussell Mar 20, 2024

absternator Mar 20, 2024 •

edited

Loading

EmmaLRussell Mar 20, 2024

absternator Mar 20, 2024

EmmaLRussell Mar 20, 2024

absternator Mar 20, 2024

absternator commented Mar 20, 2024

EmmaLRussell Mar 25, 2024

EmmaLRussell left a comment

EmmaLRussell Mar 25, 2024 •

edited

Loading

absternator Mar 25, 2024

		this.project.status = { assign: "submitted", microreact: "submitted", network: "submitted" };
		this.pollAnalysisStatus();

	this.toast.showErrorToast("Error fetching analysis status. Try refresh page, or create a new project.");
	this.toast.showErrorToast("Error fetching analysis status. Try refreshing the page, or create a new project.");

Bacpop-163 Error handle + delete sample #60

Bacpop-163 Error handle + delete sample #60

Conversation

absternator commented Mar 18, 2024

codecov bot commented Mar 18, 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

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

absternator Mar 20, 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

absternator commented Mar 20, 2024

Choose a reason for hiding this comment

EmmaLRussell left a comment

Choose a reason for hiding this comment

EmmaLRussell Mar 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Mar 18, 2024 •

edited

Loading

absternator Mar 20, 2024 •

edited

Loading

EmmaLRussell Mar 25, 2024 •

edited

Loading