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

Allow records to be updated without inserting new attachments #3130

Merged
merged 19 commits into from
Jan 23, 2024
Merged

Allow records to be updated without inserting new attachments #3130

merged 19 commits into from
Jan 23, 2024

Conversation

alexcottner
Copy link
Contributor

@alexcottner alexcottner commented Jan 18, 2024

Fixes #3128 by allowing records to be updated (via JSON or normal form) without requiring attachments to be updated.

Logic changes

  • Users can now change properties without having to upload a new file when editing records.
  • Users will not longer accidentally delete an attachment when editing the JSON data.

Test changes

  • Several new tests for RecordForm, as we only had basic testing through RecordAttributes_test before.
    • I considered refactoring these components and tests to be cleaner.
    • That feels excessive for this PR and it feels okay to have basic tests at the higher level component and more aggressive tests at the lower level component.
  • Added a new test to verify that JSONRecordForm returns the previous attachment data when saving.

Note: This is forked from #3127 .

Copy link
Contributor

@leplatrem leplatrem left a comment

Choose a reason for hiding this comment

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

Sorry I realized too late it was on draft 😬

} = collection;
const attachmentConfig = {
enabled: attachment?.enabled,
required: attachment?.required && !record?.data?.attachment
Copy link
Contributor

Choose a reason for hiding this comment

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

Shall we add a comment here making it explicit that this allows records to be edited without attachments to be reuploaded?

test/components/record/RecordForm_test.jsx Show resolved Hide resolved
}
}} />);
expect(result.queryByLabelText("TestTitle").disabled).toBe(true);
expect(result.queryByLabelText("TestContent").disabled).toBe(true);
Copy link
Contributor

Choose a reason for hiding this comment

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

No expectation about file form?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In that scenario we weren't rendering the file form, but I can switch it so we are and check it too.

@alexcottner alexcottner marked this pull request as ready for review January 19, 2024 18:47
"publish-to-gh-pages": "cross-env ASSET_PATH=/kinto-admin/ npm run build && gh-pages --add --dist build/",
"publish-to-gh-pages": "ASSET_PATH=/kinto-admin/ npm run build && gh-pages --add --dist build/",
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we still need cross-env? I guess only for running this command on Windows?

But also, maybe this is something we should migrate to a Github Actions https://github.com/actions/deploy-pages

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Started working on migrating this to a github action in another draft PR. I don't think we should bother with cross-env.

@alexcottner alexcottner merged commit 957cafe into Kinto:master Jan 23, 2024
6 checks passed
@alexcottner alexcottner deleted the issue-3128 branch February 6, 2024 19:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The "Edit Raw JSON" UI does not respect "Attachment required"
3 participants