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.

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

fix: GEO-1088 and GEO 1153 Display error message when download fails and fix disappearing file on update #776

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ vi.mock('../../services/notificationService', () => ({
let store: ReturnType<typeof useAnnouncementSelectionStore>;
describe('EditAnnouncementPage', () => {
beforeEach(() => {
vi.clearAllMocks();
setActivePinia(pinia);
store = useAnnouncementSelectionStore();
store.reset();
Expand Down Expand Up @@ -112,6 +113,44 @@ describe('EditAnnouncementPage', () => {
});

describe('when publishing announcement', () => {
describe('when expires on is before active on', () => {
it('should show error message', async () => {
const publishDate = LocalDate.now().plusDays(4);
store.setAnnouncement({
title: 'title',
description: 'description',
active_on: convert(publishDate).toDate(),
expires_on: convert(publishDate.minusDays(1)).toDate(),
status: 'DRAFT',
announcement_resource: [],
} as any);
const { getByRole, getByText } = await wrappedRender();
const saveButton = getByRole('button', { name: 'Save' });
await fireEvent.click(saveButton);
// await fireEvent.click(publishButton);
await waitFor(() => {
expect(
getByText('Publish On date should be before expiry date.'),
).toBeVisible();
});
});
});
describe('when active on date is not set', () => {
it('should show error message', async () => {
store.setAnnouncement({
title: 'title',
description: 'description',
status: 'PUBLISHED',
announcement_resource: [],
} as any);
const { getByRole, getByText } = await wrappedRender();
const saveButton = getByRole('button', { name: 'Save' });
await fireEvent.click(saveButton);
await waitFor(() => {
expect(getByText('Active On date is required.')).toBeVisible();
});
});
});
it('should require Active On date to be not in the past', async () => {
store.setAnnouncement({
title: 'title',
Expand Down Expand Up @@ -242,6 +281,7 @@ describe('EditAnnouncementPage', () => {
title: 'title',
description: 'description',
active_on: new Date(),
expires_on: convert(LocalDate.now().plusDays(1)).toDate(),
status: 'PUBLISHED',
announcement_resource: [
{
Expand All @@ -259,6 +299,19 @@ describe('EditAnnouncementPage', () => {
await waitFor(() => {
expect(fileNameInput).toHaveValue('');
});
const saveButton = getByRole('button', { name: 'Save' });
await fireEvent.click(saveButton);
await waitFor(async () => {
const confirmButton = getByRole('button', { name: 'Confirm' });
await fireEvent.click(confirmButton);
});
await waitFor(() => {
const args = mockUpdateAnnouncement.mock.calls[0];
const data = args[1];
expect(data.fileDisplayName).toBeUndefined();
expect(data.attachment).toBeUndefined();
expect(data.attachmentId).toBeUndefined();
});
});
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -573,6 +573,7 @@ const handleDeleteLink = () => {
};

const handleDeleteFile = () => {
attachment.value = undefined;
fileDisplayName.value = undefined;
fileDisplayOnly.value = false;
};
Expand Down Expand Up @@ -841,6 +842,10 @@ const handleSave = handleSubmit(async (values) => {
linkUrl: isEmpty(values.linkUrl) ? undefined : values.linkUrl,
status: status.value,
attachment: attachment.value,
attachmentId:
!values.attachment && !values.fileDisplayName
? undefined
: values.attachmentId,
});
});
</script>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ import { sanitizeUrl } from '@braintree/sanitize-url';
import ApiService from '../../services/apiService';
import { saveAs } from 'file-saver';
import { NotificationService } from '../../services/notificationService';
import { FILE_DOWNLOAD_ERROR } from '../../constants';

defineProps<{
announcement: Announcement;
Expand All @@ -67,11 +68,7 @@ async function downloadAnnouncementResource(
);
} catch (error) {
console.error(error);
NotificationService.pushNotificationError(
'There is a problem with this link/file, please try again later or contact the helpdesk.',
'',
30000,
);
NotificationService.pushNotificationError(FILE_DOWNLOAD_ERROR, '', 30000);
}
} else if (announcementResource.announcement_resource_file) {
//When a resource with type ATTACHMENT hasn't yet been uploaded to the
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<template>
<div class="root">
<a @click="ApiService.downloadFile(id)">
<a :aria-label="name" @click="downloadFile(id)">
{{ name }}
</a>
<v-btn
Expand All @@ -21,6 +21,8 @@
<script setup lang="ts">
import { defineProps } from 'vue';
import ApiService from '../../services/apiService';
import { NotificationService } from '../../services/notificationService';
import { FILE_DOWNLOAD_ERROR } from '../../constants';

interface AttachmentResourceProps {
id: string;
Expand All @@ -29,6 +31,15 @@ interface AttachmentResourceProps {
const { id, name } = defineProps<AttachmentResourceProps>();

const emits = defineEmits(['onEdit', 'onDelete']);

const downloadFile = async (id: string) => {
try {
await ApiService.downloadFile(id);
} catch (error) {
console.error(error);
NotificationService.pushNotificationError(FILE_DOWNLOAD_ERROR, '', 30000);
}
};
</script>
<style scoped lang="scss">
.root {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
import { fireEvent, render } from '@testing-library/vue';
import AttachmentResource from '../AttachmentResource.vue';
import { describe, it, expect, vi } from 'vitest';
import { FILE_DOWNLOAD_ERROR } from '../../../constants';

const mockDownloadFile = vi.fn();

vi.mock('../../../services/apiService', () => ({
default: {
downloadFile: (...args) => mockDownloadFile(...args),
},
}));

const pushNotificationErrorMock = vi.fn();
vi.mock('../../../services/notificationService', () => ({
NotificationService: {
pushNotificationError: (...args) => pushNotificationErrorMock(...args),
},
}));

describe('AttachmentResource', () => {
it('should download file', async () => {
const attachment = {
id: 1,
name: 'Test file',
};
const { getByLabelText } = await render(AttachmentResource, {
props: { ...attachment },
});
const link = await getByLabelText('Test file');
expect(link).toBeInTheDocument();
await fireEvent.click(link);
await expect(mockDownloadFile).toHaveBeenCalledWith(1);
});

it('should display error message when download fails', async () => {
mockDownloadFile.mockRejectedValue(new Error('Download failed'));
const attachment = {
id: 1,
name: 'Test file',
};
const { getByLabelText } = await render(AttachmentResource, {
props: { ...attachment },
});
const link = await getByLabelText('Test file');
expect(link).toBeInTheDocument();
await fireEvent.click(link);
await expect(pushNotificationErrorMock).toHaveBeenCalledWith(FILE_DOWNLOAD_ERROR, '', 30000);
});
});
3 changes: 3 additions & 0 deletions admin-frontend/src/constants/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,3 +17,6 @@ export const RoleOptions = [
{ label: RoleLabels['PTRT-ADMIN'], value: 'PTRT-ADMIN' },
{ label: RoleLabels['PTRT-USER'], value: 'PTRT-USER' },
];

export const FILE_DOWNLOAD_ERROR =
'There is a problem with this link/file, please try again later or contact the helpdesk.';
2 changes: 2 additions & 0 deletions admin-frontend/src/services/__tests__/apiService.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -462,6 +462,8 @@ describe('ApiService', () => {
linkUrl: 'https://example.com',
linkDisplayName: 'example',
attachment: 'test',
fileDisplayName: 'test',
attachmentId: '1',
};
const mockResponse = {
data: {},
Expand Down
10 changes: 8 additions & 2 deletions admin-frontend/src/services/apiService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -534,9 +534,15 @@ const buildFormData = (data: AnnouncementFormValue) => {
}

if (data.attachment) {
formData.append('fileDisplayName', data.fileDisplayName!);
formData.append('attachmentId', data.attachmentId!);
formData.append('file', data.attachment);
}

if (data.fileDisplayName) {
formData.append('fileDisplayName', data.fileDisplayName);
}

if (data.attachmentId) {
formData.append('attachmentId', data.attachmentId);
}
return formData;
};
4 changes: 1 addition & 3 deletions backend/src/v1/routes/announcement-routes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ import {
PatchAnnouncementsSchema,
PatchAnnouncementsType,
} from '../types/announcements';
import { omit } from 'lodash';
import { DateTimeFormatter, ZonedDateTime } from '@js-joda/core';

const router = Router();
Expand Down Expand Up @@ -159,8 +158,7 @@ router.put(
// Create announcement
const announcement = await updateAnnouncement(
req.params.id,
/* istanbul ignore next */
file ? data : omit(data, 'attachmentId'),
data,
user.admin_user_id,
);
return res.json(announcement);
Expand Down