-
Notifications
You must be signed in to change notification settings - Fork 3
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.
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
Site Image Deletion #303
Site Image Deletion #303
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work guys!
const deleteImage = (imageId: number): Promise<void> => { | ||
return AppAxiosInstance.delete( | ||
ParameterizedApiRoutes.DELETE_IMAGE(imageId), | ||
).then((res) => res.data); | ||
}; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
⛏️ Could we add a couple tests for the new route here in protectedApiClient.test.ts? These tests are mostly just here as consistency checks to make sure we don't accidentally break a route in the future.
src/components/careEntry/index.tsx
Outdated
import { | ||
EditButton, | ||
StyledClose, | ||
} from '../themedComponents'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
⛏️ Make sure to reformat this to fit the style check - I think the proper formatting would be import { EditButton, StyledClose } from '../themedComponents';
function onClickDeleteImage(imageId: number) { | ||
protectedApiClient.deleteImage(imageId).then((ok) => { | ||
message.success('success'); | ||
setShowDeleteForm(false); | ||
}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
⛏️ Can we call dispatch()
in this function as well so the page refreshes with our changes after we delete an image? There's a good example of this in onClickDeleteActivity()
function in our CareEntry
component that demonstrates how to do this.
Also, could we change the success message to something a little more descriptive, maybe something like "Image successfully deleted!"?
<div> | ||
<DeleteSiteImageButton | ||
type="primary" | ||
onClick={() => setShowDeleteForm(!showDeleteForm)} | ||
> | ||
Delete | ||
</DeleteSiteImageButton> | ||
</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
⛏️ Can we make it so this button only shows up for admins and the user who uploaded the image? The delete site image route is only callable by admins and the image's uploader - we probably shouldn't give other users this option if it won't work for them.
The image uploader's ID is available from the image, and we can get it the same way we get the image's uploader username or upload date (this field might need to be added to the SiteEntryImage
type)
onConfirm: () => void; | ||
} | ||
|
||
const ConfirmationModel: React.FC<ConfirmationModelProps> = ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔧 Quick nitpick - did you mean "Modal", not "Model"?
title, | ||
onConfirm, | ||
}) => { | ||
return ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯
Duplicated Here: #310 |
Checklist
yarn run check
yarn run test
Why
Resolves #288
This ticket adds the delete button to site images, allowing users to delete uploaded images. This keeps our site clean, to avoid irrelevant and incorrect images from staying online.
This PR
This change both adds delete functionality to the client, but adds a UI component to actually add an image. This deletion occurs both in AWS and the database (as dictated by the backend)
Screenshots
Verification Steps
We manually tested this by: