#285 delete application in draft #974

nmadruga · 2021-09-09T06:27:25Z

Fixes #285

@adamdewey That one is for you : )

Changes

Add confirmation and message to show before deletion
Already displayed under the switch that checks it's a draft application should check anything else?
[Needs checking] Should be considering the current user's row level permissions - so can only delete if it's yours

adamdewey · 2021-09-09T07:41:12Z

Oh! 👏

CarlosNZ · 2021-09-13T04:52:02Z

src/utils/messages.ts

+  APPLICATION_DELETION_CONFIRM: {
+    title: 'Delete application',
+    message: 'Please confirm you would like to delete a draft application',
+    option: 'OK',
+  },


Prob put this in constants.ts since we have to get rid of messages when we do localisation functionality?

Ok - checked PR #1004 and both files messages and `constants are deleted. So I will leave here for now.

Yeah, that's cool, I'll need to do a big mop-up before #1004 can merged, so leave any constants stuff for now.

CarlosNZ · 2021-09-13T05:10:22Z

src/components/List/ApplicationsList.tsx

+
+  if (error) return <Message error header={strings.ERROR_APPLICATION_DELETE} list={[error]} />
+
+  return isDeleted ? null : (


Is there a better way to do this? From what I understand, this just set the local component to "deleted" and just hides it from UI even though it's still actually in the list. What we really want to do is trigger a re-fetch in the list, which means you won't need any state in here.

One way to do it, maybe:
Use "refetch" in "ListWrapper": {data, loading, error: applicationsError, refetch } = useGetApplicationListQuery(...

And pass this down as "refetchList" or something, then in the ApplicationRow component, use the delete mutation's "ON_COMPLETED" property, and call "refetchList" on completion (so no need for useEffect either).

(One side effect of the way you've done it, is if the list is longer than one page, then when you delete one, the list won't re-paginate, which is confusing cos it looks like you've got less applications that there are -- in fact, you could delete all the applications on a page and the user would think they've deleted everything because the existing ones haven't rippled round to the current page like they should have).

Doing this change now. Thanks : )

CarlosNZ · 2021-09-13T20:38:07Z

Actually, now I think about this, the actual deletion should probably be through a back-end endpoint, for security reasons. I know you check for Draft status here, but that's all UI-based -- it's still easy to send "delete" queries to GraphQL for applications not in Draft.

I think we should probably set policy so no user has "delete" permissions on any table directly, only the "isAdmin" JWT in the back-end. So the front-end would make request for deletion, and the back-end does all the checks, then handles the deletion, and returns a confirmation to front-end, which then knows to refetch the list.

Also -- the back-end needs to delete any files associated with the application too.

I realise that makes this a somewhat bigger job, but leaving it like this is quite a big security hole -- so will need to make a new issue and add it to this issue please.

(I can probably do the back-end after I finish up on Outcomes, so maybe leave this in PR for now, and come back to it when the back-end API is available)

nmadruga · 2021-09-16T03:01:50Z

Actually, now I think about this, the actual deletion should probably be through a back-end endpoint, for security reasons. I know you check for Draft status here, but that's all UI-based -- it's still easy to send "delete" queries to GraphQL for applications not in Draft.

I think we should probably set policy so no user has "delete" permissions on any table directly, only the "isAdmin" JWT in the back-end. So the front-end would make request for deletion, and the back-end does all the checks, then handles the deletion, and returns a confirmation to front-end, which then knows to refetch the list.

Also -- the back-end needs to delete any files associated with the application too.

I realise that makes this a somewhat bigger job, but leaving it like this is quite a big security hole -- so will need to make a new issue and add it to this issue please.

(I can probably do the back-end after I finish up on Outcomes, so maybe leave this in PR for now, and come back to it when the back-end API is available)

Yes, good point. What I was thinking is that back-end would already be preventing that only applications related to your applications (already in place) and in the status draft (need to add as part of this-sprint-issue #119), so this check in UI is only for showing the option - not preventing the deletion on the back-end. But you are right, we need to add this restriction on the back-end to make sure there is no hole on this end. If you would like to have a go on changing the policies to check that today I think that should be enough? And the deletion of the file coud be done as part of the garbage-collection issue:
So files that are associated to non-existing applications they get removed?

nmadruga · 2021-09-23T00:04:26Z

@CarlosNZ planning to have a look on adding some restrictions to users as to only delete application in status = DRAFT or WITHDRAWN (if we will still do this one) or EXPIRED. I don't think we should allow changing if status CHANGES_REQUIRED.

Have you and @andreievg started some discussions around security and row-level permissions?

CarlosNZ · 2021-09-23T00:09:29Z

Have you and @andreievg started some discussions around security and row-level permissions?

Kind of, you'll see I've updated the linked issues on this summary issue with more details as to what we discussed. I'll make a comment about deletion on the Policies issue.

The main thing to fix in this PR I think is just my first comment, as I don't think it's good practice to just hide (using CSS) items from the table without actually removing them from the list state -- seems likely to lead to unexpected bugs (such as the one I mentioned in that comment)

nmadruga · 2021-09-26T23:14:37Z

Ok - Changes done on Front-end.

Regarding security issue, should be dealt as part of issue msupply-foundation/conforma-server#589. Have added an comment there about that now.

CarlosNZ · 2021-09-26T23:16:01Z

Ok - Changes done on Front-end.

Cool, will check out shortly

CarlosNZ

Great, thanks. I'm pleased the refetch/OnCompleted mechanism worked out.

Works well in the UI now, applications from later pages wrap around correctly when deleting.

Can you just remove those two unused hooks in ApplicationsList.

👍

CarlosNZ · 2021-09-27T00:51:34Z

src/components/List/ApplicationsList.tsx

@@ -1,6 +1,8 @@
-import React, { Fragment } from 'react'
+import React, { Fragment, useEffect, useState } from 'react'


useEffect, useState not used

nmadruga · 2021-09-27T22:31:56Z

Works well in the UI now, applications from later pages wrap around correctly when deleting.

Can you just remove those two unused hooks in ApplicationsList.

Sure. Thank you!

nmadruga added 3 commits September 9, 2021 18:23

Add ability to delete application with status draft

ae77a49

Add ability to delete application with status draft

f8d36ba

Remove unused imports

684d2a1

CarlosNZ requested changes Sep 13, 2021

View reviewed changes

nmadruga added the #toDemo label Sep 23, 2021

CarlosNZ added #inDemo and removed #toDemo labels Sep 23, 2021

nmadruga added 4 commits September 27, 2021 11:32

Merge branch 'develop' into #285-delete-application-in-draft

47a1818

Merge branch 'develop' into #285-delete-application-in-draft

e8e958c

Replace using state to use refetch of list after deletion

f4f7dff

Remove warning

b9d1fb8

nmadruga requested a review from CarlosNZ September 26, 2021 23:14

CarlosNZ approved these changes Sep 27, 2021

View reviewed changes

Remove unused imports

4b07077

nmadruga merged commit 43373f2 into develop Sep 27, 2021

CarlosNZ deleted the #285-delete-application-in-draft branch November 2, 2022 00:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#285 delete application in draft #974

#285 delete application in draft #974

nmadruga commented Sep 9, 2021 •

edited

Loading

adamdewey commented Sep 9, 2021

CarlosNZ Sep 13, 2021

nmadruga Sep 26, 2021

CarlosNZ Sep 26, 2021

CarlosNZ Sep 13, 2021

nmadruga Sep 26, 2021

CarlosNZ commented Sep 13, 2021

nmadruga commented Sep 16, 2021

nmadruga commented Sep 23, 2021

CarlosNZ commented Sep 23, 2021

nmadruga commented Sep 26, 2021

CarlosNZ commented Sep 26, 2021

CarlosNZ left a comment

CarlosNZ Sep 27, 2021

nmadruga commented Sep 27, 2021


		if (error) return <Message error header={strings.ERROR_APPLICATION_DELETE} list={[error]} />

		return isDeleted ? null : (

		@@ -1,6 +1,8 @@
		import React, { Fragment } from 'react'
		import React, { Fragment, useEffect, useState } from 'react'

#285 delete application in draft #974

#285 delete application in draft #974

Conversation

nmadruga commented Sep 9, 2021 • edited Loading

Changes

adamdewey commented Sep 9, 2021

CarlosNZ Sep 13, 2021

Choose a reason for hiding this comment

nmadruga Sep 26, 2021

Choose a reason for hiding this comment

CarlosNZ Sep 26, 2021

Choose a reason for hiding this comment

CarlosNZ Sep 13, 2021

Choose a reason for hiding this comment

nmadruga Sep 26, 2021

Choose a reason for hiding this comment

CarlosNZ commented Sep 13, 2021

nmadruga commented Sep 16, 2021

nmadruga commented Sep 23, 2021

CarlosNZ commented Sep 23, 2021

nmadruga commented Sep 26, 2021

CarlosNZ commented Sep 26, 2021

CarlosNZ left a comment

Choose a reason for hiding this comment

CarlosNZ Sep 27, 2021

Choose a reason for hiding this comment

nmadruga commented Sep 27, 2021

nmadruga commented Sep 9, 2021 •

edited

Loading