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

Edit Dialog for Images #1151

Open
wants to merge 19 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
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
7 changes: 4 additions & 3 deletions cypress/e2e/with_mock_data/items.cy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -909,6 +909,7 @@ describe('Items', () => {
.should('be.visible')
.within(() => {
cy.findByLabelText('File Name *').clear();
cy.findByText('.png').should('exist');
cy.findByLabelText('File Name *').type('test file');

cy.findByLabelText('Title').clear();
Expand All @@ -930,12 +931,12 @@ describe('Items', () => {
expect(patchRequests.length).equal(1);
const request = patchRequests[0];
expect(JSON.stringify(await request.json())).equal(
'{"file_name":"test file","description":"test description","title":"test title"}'
'{"file_name":"test file.png","description":"test description","title":"test title"}'
);
});
});

it('not changing any fields shows error', () => {
it('shows error when no fields have been changed', () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
it('shows error when no fields have been changed', () => {
it('shows error message when no fields have been changed', () => {

apologies i know i changed it to this the first time :)

cy.findByText('5YUQDDjKpz2z').click();
cy.findByText(
'High-resolution cameras for beam characterization. 1'
Expand All @@ -957,7 +958,7 @@ describe('Items', () => {
cy.findByRole('button', { name: 'Save' }).should('be.disabled');
});

it('Required fields that are cleared are not allowed and show error message', () => {
it('shows error message when required fields are cleared', () => {
cy.findByText('5YUQDDjKpz2z').click();
cy.findByText(
'High-resolution cameras for beam characterization. 1'
Expand Down
12 changes: 8 additions & 4 deletions src/common/editFileDialog.component.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,10 @@ describe('Edit file dialog', () => {

it('Edits an image correctly', async () => {
createView();

//Checks if file extension is displayed. If it's editable, actual value will not match expected.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
//Checks if file extension is displayed. If it's editable, actual value will not match expected.
// Checks if file extension is displayed. If it's editable, actual value will not match expected.

expect(screen.getByText('.png')).toBeInTheDocument();

modifyFileValues({
file_name: 'test_file_name.jpeg',
title: 'Test Title',
Expand All @@ -98,15 +102,15 @@ describe('Edit file dialog', () => {
await user.click(saveButton);

expect(axiosPatchSpy).toHaveBeenCalledWith('/images/1', {
file_name: 'test_file_name.jpeg',
file_name: 'test_file_name.jpeg.png',
title: 'Test Title',
description: 'Test Description',
});

expect(onClose).toHaveBeenCalled();
});

it('No values changed shows correct error message', async () => {
it('shows correct error message when no values are changed', async () => {
createView();

const saveButton = screen.getByRole('button', { name: 'Save' });
Expand All @@ -120,7 +124,7 @@ describe('Edit file dialog', () => {
).toBeInTheDocument();
});

it('Required fields show error if they are whitespace or current value just removed', async () => {
it('shows error message if required fields are whitespace or their current value was removed', async () => {
createView();
modifyFileValues({
file_name: '',
Expand All @@ -133,7 +137,7 @@ describe('Edit file dialog', () => {
expect(onClose).not.toHaveBeenCalled();
});

it('CatchAllError request works correctly and displays refresh page message', async () => {
it('displays refresh page message and a CatchAllError request works correctly', async () => {
createView();
modifyFileValues({
file_name: 'Error 500',
Expand Down
44 changes: 35 additions & 9 deletions src/common/editFileDialog.component.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
DialogTitle,
FormHelperText,
Grid,
InputAdornment,
TextField,
} from '@mui/material';
import { useForm } from 'react-hook-form';
Expand Down Expand Up @@ -44,14 +45,25 @@

const { mutateAsync: patchFile, isPending: isEditPending } = usePatchFile();

const point = selectedFile?.file_name?.lastIndexOf('.') ?? 0;
const extension = selectedFile?.file_name?.slice(point) ?? '';
const initialName = selectedFile?.file_name?.slice(0, point) ?? '';

console.log(extension);

const selectedFileCopy: ObjectFilePatch = React.useMemo(
() => (selectedFile ? { ...selectedFile, file_name: initialName } : {}),
[selectedFile, initialName]
);

const initialFile: ObjectFilePatch = React.useMemo(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you make the same changes that you have do to the uppy post here please. So don't allow the user to change the extenstion. and display the extension next the textField please.

() =>
selectedFile ?? {
selectedFileCopy ?? {
file_name: '',
title: '',
description: '',
},

Check warning on line 65 in src/common/editFileDialog.component.tsx

View check run for this annotation

Codecov / codecov/patch

src/common/editFileDialog.component.tsx#L62-L65

Added lines #L62 - L65 were not covered by tests
[selectedFile]
[selectedFileCopy]
);

const {
Expand Down Expand Up @@ -89,16 +101,18 @@
const handleEditFile = React.useCallback(
(fileData: ObjectFilePatch) => {
if (selectedFile) {
const isFileNameUpdated = fileData.file_name !== selectedFile.file_name;
const isFileNameUpdated =
fileData.file_name !== selectedFileCopy.file_name;

const isDescriptionUpdated =
fileData.description !== selectedFile.description;
fileData.description !== selectedFileCopy.description;

const isTitleUpdated = fileData.title !== selectedFile.title;
const isTitleUpdated = fileData.title !== selectedFileCopy.title;

let fileToEdit: ObjectFilePatch = {};
const fileToEdit: ObjectFilePatch = {};

if (isFileNameUpdated) fileToEdit.file_name = fileData.file_name;
if (isFileNameUpdated)
fileToEdit.file_name = fileData.file_name + extension;
if (isDescriptionUpdated) fileToEdit.description = fileData.description;
if (isTitleUpdated) fileToEdit.title = fileData.title;

Expand All @@ -119,11 +133,18 @@
}
}
},
[selectedFile, patchFile, handleClose, setError]
[
selectedFile,
selectedFileCopy,
extension,
patchFile,
handleClose,
setError,
]
);

return (
<Dialog open={open} maxWidth="lg" fullWidth>
<Dialog open={open} maxWidth="sm" fullWidth>
<DialogTitle>{`Edit ${fileType}`}</DialogTitle>
<DialogContent>
<Grid container direction="column" spacing={1} component="form">
Expand All @@ -135,6 +156,11 @@
{...register('file_name')}
error={!!errors.file_name}
helperText={errors.file_name?.message}
InputProps={{
endAdornment: (
<InputAdornment position="end">{extension}</InputAdornment>
),
}}
fullWidth
/>
</Grid>
Expand Down
2 changes: 1 addition & 1 deletion src/mocks/handlers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1084,7 +1084,7 @@ export const handlers = [

const fullBody = { ...obj, ...body };

if (fullBody.file_name === 'Error 500') {
if (fullBody.file_name === 'Error 500.png') {
return HttpResponse.json(
{ detail: 'Something went wrong' },
{ status: 500 }
Expand Down
Loading