From 2fa46760b0ddc70781119db7846b5730b6046a86 Mon Sep 17 00:00:00 2001 From: Jonathan ARNAULT Date: Fri, 7 Feb 2025 14:26:56 +0100 Subject: [PATCH] Feat(annotation): Add annotation deletion support --- cypress/e2e/phase_1/admin_display.cy.js | 4 + cypress/e2e/phase_4/annotation.cy.js | 125 +++++++++++++++- cypress/support/annotation.js | 7 +- src/api/controller/api/annotation.js | 52 +++++-- src/api/controller/api/annotation.spec.js | 82 ++++++++++- src/api/models/annotation.js | 19 ++- src/api/models/annotation.spec.js | 38 +++++ src/app/custom/translations.tsv | 16 +- .../details/AnnotationDeleteButton.js | 68 +++++++++ .../details/AnnotationDeleteButton.spec.js | 137 ++++++++++++++++++ .../annotations/details/AnnotationForm.js | 21 +-- .../annotations/hooks/useDeleteAnnotation.js | 55 +++++++ .../js/annotation/fields/AuthorEmailField.js | 7 +- 13 files changed, 593 insertions(+), 38 deletions(-) create mode 100644 src/app/js/admin/annotations/details/AnnotationDeleteButton.js create mode 100644 src/app/js/admin/annotations/details/AnnotationDeleteButton.spec.js create mode 100644 src/app/js/admin/annotations/hooks/useDeleteAnnotation.js diff --git a/cypress/e2e/phase_1/admin_display.cy.js b/cypress/e2e/phase_1/admin_display.cy.js index a4fbf0d2c..75d979fe3 100644 --- a/cypress/e2e/phase_1/admin_display.cy.js +++ b/cypress/e2e/phase_1/admin_display.cy.js @@ -71,6 +71,10 @@ describe('Home Page', () => { cy.findByRole('gridcell', { name: /newField/, timeout: 1000, + }).trigger('mouseenter'); + + cy.findByRole('button', { + name: /edit-newField/, }).click(); cy.get(`button[value="home"]`).should('not.have.class', 'Mui-selected'); diff --git a/cypress/e2e/phase_4/annotation.cy.js b/cypress/e2e/phase_4/annotation.cy.js index bea8f0e2a..aee71ed67 100644 --- a/cypress/e2e/phase_4/annotation.cy.js +++ b/cypress/e2e/phase_4/annotation.cy.js @@ -140,7 +140,7 @@ describe('Annotation', () => { describe('resources', () => { describe('create list and update an Annotation', () => { - it.only('should create an annotation on resource field', () => { + it('should create an annotation on resource field', () => { cy.findByText('Search').click(); searchDrawer.search('Terminator 2'); searchDrawer.waitForLoading(); @@ -181,7 +181,7 @@ describe('Annotation', () => { 'Internal Comment', 'Administrator', 'Contributor', - 'Comment', + 'Contributor Comment', 'Submission date', 'Last modified on', ]); @@ -284,7 +284,9 @@ describe('Annotation', () => { 'exist', ); - cy.findByText('Annotations').click(); + cy.findByRole('link', { + name: 'Cancel', + }).click(); cy.wait(1000); cy.findByRole('progressbar').should('not.exist'); @@ -336,6 +338,123 @@ describe('Annotation', () => { name: `Add an annotation to Liste des films field`, }).should('not.exist'); }); + + it('should delete an annotation on resource field', () => { + cy.findByText('Search').click(); + searchDrawer.search('Terminator 2'); + searchDrawer.waitForLoading(); + cy.findByTitle('Terminator 2').click(); + annotation.createTitleAnnotation({ + fieldLabel: 'actors', + comment: 'This is a comment', + authorName: 'John Doe', + authorEmail: 'john.doe@example.org', + }); + + cy.findByText('Search').click(); + searchDrawer.search('RoboCop'); + searchDrawer.waitForLoading(); + cy.findByTitle('RoboCop').click(); + annotation.createTitleAnnotation({ + fieldLabel: 'rating', + comment: 'This is another comment', + authorName: 'Jane Smith', + authorEmail: 'jane.smith@example.org', + }); + cy.findByText('More').click(); + menu.goToAdminDashboard(); + cy.findByText('Annotations').click(); + + cy.findAllByRole('cell').then((cells) => { + const firstUri = cells[0].textContent; + const secondUri = cells[14].textContent; + + expect(firstUri).to.match(/uid:\//); + expect(secondUri).to.match(/uid:\//); + + expect( + cells.toArray().map((cell) => cell.textContent), + ).to.deep.equal([ + firstUri, + 'RoboCop', + 'rating', + '[bZE+]', + '', + '', + '', + 'To Review', + '', + '', + 'Jane Smith', + 'This is another comment', + new Date().toLocaleDateString(), + new Date().toLocaleDateString(), + secondUri, + 'Terminator 2', + 'actors', + '[K8Lu]', + '', + '', + '', + 'To Review', + '', + '', + 'John Doe', + 'This is a comment', + new Date().toLocaleDateString(), + new Date().toLocaleDateString(), + ]); + }); + + cy.findByText('Terminator 2').click(); + + cy.findByRole('heading', { + name: /^Annotation: uid:\//, + }).should('be.visible'); + cy.findByRole('heading', { + name: 'Terminator 2', + }).should('be.visible'); + + cy.findByRole('button', { + name: 'Delete the annotation', + }).click(); + + cy.findByRole('button', { + name: 'Delete', + }).click(); + + cy.findByRole('alert', {}).should( + 'have.text', + 'The annotation has been deleted.', + ); + + cy.wait(500); + + cy.findAllByRole('cell').then((cells) => { + const firstUri = cells[0].textContent; + + expect(firstUri).to.match(/uid:\//); + + expect( + cells.toArray().map((cell) => cell.textContent), + ).to.deep.equal([ + firstUri, + 'RoboCop', + 'rating', + '[bZE+]', + '', + '', + '', + 'To Review', + '', + '', + 'Jane Smith', + 'This is another comment', + new Date().toLocaleDateString(), + new Date().toLocaleDateString(), + ]); + }); + }); }); }); diff --git a/cypress/support/annotation.js b/cypress/support/annotation.js index 0b8d752ab..0e4e654f8 100644 --- a/cypress/support/annotation.js +++ b/cypress/support/annotation.js @@ -11,9 +11,10 @@ function createAnnotation({ buttonLabel, comment, authorName, authorEmail }) { cy.wait(350); - cy.findByRole('textbox', { name: 'Name *', timeout: 1500 }).type( - authorName, - ); + cy.findByRole('textbox', { + name: 'Last name, First name *', + timeout: 1500, + }).type(authorName); if (authorEmail) { cy.findByRole('textbox', { name: /Email/, timeout: 1500 }).type( diff --git a/src/api/controller/api/annotation.js b/src/api/controller/api/annotation.js index d913484d0..a01a986d0 100644 --- a/src/api/controller/api/annotation.js +++ b/src/api/controller/api/annotation.js @@ -246,18 +246,8 @@ export async function getAnnotations(ctx) { })), }; } -/** - * @param {Koa.Context} ctx - */ -export async function getAnnotation(ctx, id) { - const annotation = await ctx.annotation.findOneById(id); - - if (!annotation) { - ctx.response.status = 404; - - return; - } +async function bindResourceAndFieldToAnnnotation(ctx, annotation) { const [titleField, subResourceTitleFields] = await Promise.all([ ctx.field.findResourceTitle(), ctx.field.findSubResourceTitles(), @@ -271,8 +261,7 @@ export async function getAnnotation(ctx, id) { ? await ctx.field.findOneById(annotation.fieldId) : null; - ctx.response.status = 200; - ctx.response.body = { + return { ...annotation, field, resource: resource @@ -288,6 +277,25 @@ export async function getAnnotation(ctx, id) { }; } +/** + * @param {Koa.Context} ctx + */ +export async function getAnnotation(ctx, id) { + const annotation = await ctx.annotation.findOneById(id); + + if (!annotation) { + ctx.response.status = 404; + + return; + } + + ctx.response.status = 200; + ctx.response.body = await bindResourceAndFieldToAnnnotation( + ctx, + annotation, + ); +} + /** * @param {Koa.Context} ctx */ @@ -312,7 +320,22 @@ export async function updateAnnotation(ctx, id) { } ctx.response.status = 200; - ctx.response.body = { data: updatedAnnotation }; + ctx.response.body = { + data: await bindResourceAndFieldToAnnnotation(ctx, updatedAnnotation), + }; +} + +export async function deleteAnnotation(ctx, id) { + const deletedCount = await ctx.annotation.deleteOneById(id); + if (deletedCount === 0) { + ctx.response.status = 404; + return; + } + + ctx.response.status = 200; + ctx.response.body = { + success: true, + }; } function getResourceTitle(resource, titleField, subResourceTitleFields) { @@ -332,6 +355,7 @@ app.use(route.get('/', getAnnotations)); app.use(route.get('/:id', getAnnotation)); app.use(koaBodyParser()); app.use(route.put('/:id', updateAnnotation)); +app.use(route.del('/:id', deleteAnnotation)); app.use(route.post('/', createAnnotation)); export default app; diff --git a/src/api/controller/api/annotation.spec.js b/src/api/controller/api/annotation.spec.js index 35a178f94..533635e88 100644 --- a/src/api/controller/api/annotation.spec.js +++ b/src/api/controller/api/annotation.spec.js @@ -4,6 +4,7 @@ import createFieldModel from '../../models/field'; import createPublishedDatasetModel from '../../models/publishedDataset'; import { createAnnotation, + deleteAnnotation, getAnnotation, getAnnotations, updateAnnotation, @@ -1336,11 +1337,27 @@ describe('annotation', () => { internalComment: 'All done', administrator: 'The Tester', updatedAt: expect.any(Date), + field: { + _id: field._id, + label: 'Annotated field', + name: 'GvaF', + position: 2, + }, + resource: { + title: 'resource title', + uri: 'uid:/1234', + }, }, }, }); expect(await annotationModel.findLimitFromSkip()).toStrictEqual([ - ctx.response.body.data, + { + ...annotation, + status: 'validated', + internalComment: 'All done', + administrator: 'The Tester', + updatedAt: expect.any(Date), + }, ]); }); @@ -1407,4 +1424,67 @@ describe('annotation', () => { ]); }); }); + + describe('DELETE /annotations/:id', () => { + let annotation; + beforeEach(async () => { + annotation = await annotationModel.create({ + resourceUri: null, + itemPath: [], + fieldId: null, + authorName: 'Developer', + authorEmail: 'developer@marmelab.com', + comment: 'This is a comment', + status: 'ongoing', + internalComment: null, + createdAt: new Date('03-01-2025'), + updatedAt: new Date('03-01-2025'), + }); + }); + + it('should succeed with a 200 if annotation does not exist', async () => { + const ctx = { + request: { + body: {}, + }, + response: {}, + annotation: annotationModel, + publishedDataset: publishedDatasetModel, + field: fieldModel, + }; + + await deleteAnnotation(ctx, annotation._id); + + expect(ctx.response).toStrictEqual({ + status: 200, + body: { + success: true, + }, + }); + + expect(await annotationModel.findLimitFromSkip()).toStrictEqual([]); + }); + + it('should fail with a 404 if annotation does not exist', async () => { + const ctx = { + request: { + body: {}, + }, + response: {}, + annotation: annotationModel, + publishedDataset: publishedDatasetModel, + field: fieldModel, + }; + + await deleteAnnotation(ctx, '404404404404404404404404'); + + expect(ctx.response).toStrictEqual({ + status: 404, + }); + + expect(await annotationModel.findLimitFromSkip()).toStrictEqual([ + annotation, + ]); + }); + }); }); diff --git a/src/api/models/annotation.js b/src/api/models/annotation.js index 3079e563b..4c04a8070 100644 --- a/src/api/models/annotation.js +++ b/src/api/models/annotation.js @@ -69,5 +69,22 @@ export default async (db) => { return annotationCollection.findOne({ _id: new ObjectId(id) }); } - return { create, updateOneById, findLimitFromSkip, count, findOneById }; + async function deleteOneById(id) { + if (!ObjectId.isValid(id)) { + return 0; + } + const { deletedCount } = await annotationCollection.deleteOne({ + _id: new ObjectId(id), + }); + return +deletedCount; + } + + return { + create, + updateOneById, + findLimitFromSkip, + count, + findOneById, + deleteOneById, + }; }; diff --git a/src/api/models/annotation.spec.js b/src/api/models/annotation.spec.js index 41edac641..5f4a99c08 100644 --- a/src/api/models/annotation.spec.js +++ b/src/api/models/annotation.spec.js @@ -247,4 +247,42 @@ describe('annotation', () => { expect(await annotationModel.findOneById('404')).toBeNull(); }); }); + + describe('deleteOneById', () => { + it('should delete an annotation', async () => { + const annotation = await annotationModel.create({ + comment: 'target annotation', + }); + + expect(await annotationModel.count({})).toBe(1); + + await expect( + annotationModel.deleteOneById(annotation._id), + ).resolves.toBe(1); + + expect(await annotationModel.count({})).toBe(0); + }); + + it('should return 0 if target annotation does not exists', async () => { + await annotationModel.create({ + comment: 'another annotation', + }); + + await expect( + annotationModel.deleteOneById('404404404404404404404404'), + ).resolves.toBe(0); + + expect(await annotationModel.count({})).toBe(1); + }); + + it('should return 0 if id is not a valid ObjectId format', async () => { + await annotationModel.create({ + comment: 'another annotation', + }); + + await expect(annotationModel.deleteOneById('404')).resolves.toBe(0); + + expect(await annotationModel.count({})).toBe(1); + }); + }); }); diff --git a/src/app/custom/translations.tsv b/src/app/custom/translations.tsv index ac854f76f..f284c9641 100644 --- a/src/app/custom/translations.tsv +++ b/src/app/custom/translations.tsv @@ -77,7 +77,7 @@ "error_composedOf.separator_invalid" "A ComposedOf separator must be a string" "Un séparateur de champ composé doit être une chaîne de caractères" "error_composedOf.fields_required" "ComposedOf must have at least two fields" "Un champ composé doit avoir au moins deux champs" "error_composedOf.fields_invalid" "ComposedOf fields must exist" "Les champs du champ composé doivent exister" -"error_completes_inexisting_target_field" "A completed field must exist" "Un champ annoté doit exister" +"error_completes_inexisting_target_field" "A captioned field must exist" "Un champ légendé doit exister" "error_label_invalid_label" "A label must be at least 2 characters long" "Une étiquette doit comporter au moins deux caractères" "error_scope_invalid_contribution_scope" "A contribution must have a cover document" "Une contribution doit comporter un document de couverture" "error_scope_invalid_scope" "A coverage must be either a dataset or a collection" "Une couverture doit être soit un jeu de données, soit une collection" @@ -1215,7 +1215,7 @@ "annotation_resource_not_found" "Resource has been removed" "La ressource a été supprimée" "annotation_field_not_found" "Field has been removed" "Le champ a été supprimé" "annotation_resource" "Resource title" "Titre de la ressource" -"annotation_comment" "Comment" "Commentaire" +"annotation_comment" "Contributor Comment" "Commentaire du contributeur" "annotation_initialValue" "Initial value" "Valeur d'origine" "annotation_authorName" "Contributor" "Contributeur" "annotation_resource_uri" "Resource URI" "URI de la ressource" @@ -1230,7 +1230,6 @@ "annotation_comment_section" "Contributor Comment" "Commentaire du contributeur" "annotation_contributor_section" "Contributor" "Contributeur" "annotation_complementary_infos_section" "Complementary Informations" "Informations complémentaires" -"annotation_author_name" "Name" "Nom" "annotation_author_email" "Email" "E-mail" "annotation_internal_comment" "Internal Comment" "Commentaire Interne" "annotation_resource_link" "Open the resource" "Ouvrir la ressource" @@ -1239,8 +1238,9 @@ "annotation.resourceId" "Resource title" "Titre de la ressource" "annotation.resource" "Resource" "Ressource" "annotation.createdAt" "Submission date" "Date de soumission" -"annotation.authorName" "Name" "Nom" -"annotation.authorEmail" "Email address (to be informed)" "Adresse e-mail (pour être informé)" +"annotation.authorName" "Last name, First name" "Nom, Prénom" +"annotation.authorEmail" "Email address" "Adresse e-mail" +"annotation.authorEmail_helpText" "The email address will allow us to contact you if needed" "L'e-mail permettra d'être contacté en cas de besoin" "annotation_status" "Status" "Statut" "annotation_internal_comment" "Internal Comment" "Commentaire interne" "annotation_administrator" "Administrator" "Gestionnaire" @@ -1249,4 +1249,8 @@ "annotation_status_ongoing" "Ongoing" "En cours" "annotation_status_validated" "Validated" "Validée" "annotation_status_rejected" "Rejected" "Refusée" -"annotation_form_title" "Edit annotation" "Modifier l'annotation" \ No newline at end of file +"annotation_form_title" "Edit annotation" "Modifier l'annotation" +"annotation_delete_button_label" "Delete the annotation" "Supprimer l'annotation" +"annotation_delete_modal_title" "Are you sure you want to delete this annotation ?" "Êtes-vous sûr de vouloir supprimer cette annotation ?" +"annotation_delete_success" "The annotation has been deleted." "L'annotation a été supprimée." +"annotation_delete_error" "An error occured while deleteing this annotation, please try again later." "Une erreur est survenue lors de la supression de l'annotation, merci de réessayer ultérieurement." \ No newline at end of file diff --git a/src/app/js/admin/annotations/details/AnnotationDeleteButton.js b/src/app/js/admin/annotations/details/AnnotationDeleteButton.js new file mode 100644 index 000000000..d7ec612c8 --- /dev/null +++ b/src/app/js/admin/annotations/details/AnnotationDeleteButton.js @@ -0,0 +1,68 @@ +import Button from '@mui/material/Button'; +import Dialog from '@mui/material/Dialog'; +import DialogTitle from '@mui/material/DialogTitle'; + +import DialogActions from '@mui/material/DialogActions'; +import PropTypes from 'prop-types'; +import React from 'react'; + +import { useTranslate } from '../../../i18n/I18NContext'; +import { useDeleteAnnotation } from '../hooks/useDeleteAnnotation'; + +export function AnnotationDeleteButton({ id, isSubmitting }) { + const { translate } = useTranslate(); + const { mutateAsync, isLoading } = useDeleteAnnotation(id); + + const [isDialogOpen, setIsDialogOpen] = React.useState(false); + + const handleOpenModal = () => { + setIsDialogOpen(true); + }; + + const handleCloseDialog = () => { + setIsDialogOpen(false); + }; + + const handleDelete = async () => { + return mutateAsync(); + }; + + return ( + <> + + + + + {translate('annotation_delete_modal_title')} + + + + + + + + ); +} + +AnnotationDeleteButton.propTypes = { + id: PropTypes.string.isRequired, + isSubmitting: PropTypes.bool.isRequired, +}; diff --git a/src/app/js/admin/annotations/details/AnnotationDeleteButton.spec.js b/src/app/js/admin/annotations/details/AnnotationDeleteButton.spec.js new file mode 100644 index 000000000..6c0c2ce63 --- /dev/null +++ b/src/app/js/admin/annotations/details/AnnotationDeleteButton.spec.js @@ -0,0 +1,137 @@ +import React from 'react'; +import { MemoryRouter } from 'react-router-dom'; + +import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; +import { fireEvent, render, waitFor } from '@testing-library/react'; +import PropTypes from 'prop-types'; +import { TestI18N } from '../../../i18n/I18NContext'; +import { useDeleteAnnotation } from '../hooks/useDeleteAnnotation'; +import { AnnotationDeleteButton } from './AnnotationDeleteButton'; + +const ANNOTATION_ID = '4f74cd4e-ecc5-4314-beb8-b98f2ce2cf61'; + +const queryClient = new QueryClient(); + +jest.mock('./../hooks/useDeleteAnnotation', () => ({ + useDeleteAnnotation: jest.fn().mockReturnValue({ + mutateAsync: jest.fn(), + isLoading: false, + }), +})); + +function TestButton({ isSubmitting = false }) { + return ( + + + + + + + + ); +} + +TestButton.propTypes = { + isSubmitting: PropTypes.bool, +}; + +describe('AnnotationDeleteButton', () => { + afterEach(() => { + jest.clearAllMocks(); + }); + + it('should delete annotation', async () => { + const mutateAsync = jest.fn(); + jest.mocked(useDeleteAnnotation).mockReturnValue({ + mutateAsync, + isLoading: false, + }); + + const wrapper = render(); + + const deleteButton = wrapper.getByRole('button', { + name: 'annotation_delete_button_label', + }); + expect(deleteButton).toBeInTheDocument(); + + await waitFor(() => { + fireEvent.click(deleteButton); + }); + + expect( + wrapper.queryByText('annotation_delete_modal_title'), + ).toBeInTheDocument(); + + await waitFor(() => { + fireEvent.click( + wrapper.getByRole('button', { + name: 'delete', + }), + ); + }); + + expect(mutateAsync).toHaveBeenCalledTimes(1); + }); + + it('should disable button when submitting', async () => { + const wrapper = render(); + + expect( + wrapper.getByRole('button', { + name: 'annotation_delete_button_label', + }), + ).toBeDisabled(); + }); + + it('should disable button when in flight', async () => { + jest.mocked(useDeleteAnnotation).mockReturnValue({ + mutateAsync: jest.fn(), + isLoading: true, + }); + + const wrapper = render(); + + expect( + wrapper.getByRole('button', { + name: 'annotation_delete_button_label', + }), + ).toBeDisabled(); + }); + + it('should not delete annotation when cancelling', async () => { + jest.mocked(useDeleteAnnotation).mockReturnValue({ + mutateAsync: jest.fn(), + isLoading: false, + }); + + const wrapper = render(); + + const deleteButton = wrapper.getByRole('button', { + name: 'annotation_delete_button_label', + }); + expect(deleteButton).toBeInTheDocument(); + + await waitFor(() => { + fireEvent.click(deleteButton); + }); + + expect( + wrapper.queryByText('annotation_delete_modal_title'), + ).toBeInTheDocument(); + + await waitFor(() => { + fireEvent.click( + wrapper.getByRole('button', { + name: 'cancel', + }), + ); + }); + + expect( + wrapper.queryByText('annotation_delete_confirm_title'), + ).not.toBeInTheDocument(); + }); +}); diff --git a/src/app/js/admin/annotations/details/AnnotationForm.js b/src/app/js/admin/annotations/details/AnnotationForm.js index 91a4c88d7..5f233052c 100644 --- a/src/app/js/admin/annotations/details/AnnotationForm.js +++ b/src/app/js/admin/annotations/details/AnnotationForm.js @@ -6,10 +6,12 @@ import Stack from '@mui/material/Stack'; import { useForm } from '@tanstack/react-form'; import PropTypes from 'prop-types'; import React from 'react'; +import { Link } from 'react-router-dom'; import { annotationUpdateSchema } from '../../../../../common/validator/annotation.validator'; import { useTranslate } from '../../../i18n/I18NContext'; import { useUpdateAnnotation } from '../hooks/useUpdateAnnotation'; +import { AnnotationDeleteButton } from './AnnotationDeleteButton'; import { AnnotationHeader } from './AnnotationHeader'; import { AnnotationInputs } from './AnnotationInputs'; import { AnnotationItems } from './AnnotationItems'; @@ -97,16 +99,17 @@ export const AnnotationForm = ({ annotation }) => { className="mui-fixed" > - + -