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

feat(core): support pasting object into array + copying individual array items #7292

Merged
merged 9 commits into from
Aug 13, 2024

Conversation

sjelfull
Copy link
Member

@sjelfull sjelfull commented Aug 1, 2024

Description

This PR fixes and adds a few things in the new copy&paste feature:

  • You may now copy a supported object type into a array field
  • You can now copy individual array items and append them into another array
  • Fixes reference validation when pasting full documents
  • Fixes flaky tests for recentSearches that became even more flaky after upgrading the React testing library

Fixes EDX-1432 and EDX-1584.

CleanShot 2024-08-08 at 14 54 53

CleanShot.2024-08-09.at.13.17.04.mp4

What to review

  • Test that this works for any sort of supported objects, like in a page builder scenario
  • Test that object types that isn't supported by a target array field won't be allowed to be pasted
  • Test that copying a single array item into another array that allows its type works

Testing

Unit tests have been added for these new scenarios.

Also new in this PR, we added unit tests for the useCopyPaste hook that covers some common scenarios. I found that only having unit tests for the underlying transferValue utility leads to false sense of security, since there is a lot that can go wrong inside the studio. This will hopefully allow us to catch bugs early when changing this logic, even if we don't have proper E2E tests setup.

Notes for release

  • Fixes an issue where you were able to copy and paste documents that contained non-existing references
  • Adds support for copying supported object types into array fields
  • Adds support for copying and pasting individual array items
  • Fixes an issue where getValueAtPath() utility wouldn't work for 0 index path segments (['array', 0])

@sjelfull sjelfull self-assigned this Aug 1, 2024
Copy link

vercel bot commented Aug 1, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
page-building-studio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 13, 2024 11:27am
performance-studio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 13, 2024 11:27am
test-compiled-studio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 13, 2024 11:27am
test-next-studio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 13, 2024 11:27am
test-studio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 13, 2024 11:27am
1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
studio-workshop ⬜️ Ignored (Inspect) Visit Preview Aug 13, 2024 11:27am

Copy link
Contributor

github-actions bot commented Aug 1, 2024

No changes to documentation

Copy link
Contributor

github-actions bot commented Aug 1, 2024

Component Testing Report Updated Aug 13, 2024 11:33 AM (UTC)

File Status Duration Passed Skipped Failed
comments/CommentInput.spec.tsx ✅ Passed (Inspect) 43s 15 0 0
formBuilder/ArrayInput.spec.tsx ✅ Passed (Inspect) 8s 3 0 0
formBuilder/inputs/PortableText/Annotations.spec.tsx ✅ Passed (Inspect) 30s 6 0 0
formBuilder/inputs/PortableText/copyPaste/CopyPaste.spec.tsx ✅ Passed (Inspect) 37s 11 7 0
formBuilder/inputs/PortableText/copyPaste/CopyPasteFields.spec.tsx ✅ Passed (Inspect) 0s 0 12 0
formBuilder/inputs/PortableText/Decorators.spec.tsx ✅ Passed (Inspect) 17s 6 0 0
formBuilder/inputs/PortableText/DisableFocusAndUnset.spec.tsx ✅ Passed (Inspect) 10s 3 0 0
formBuilder/inputs/PortableText/DragAndDrop.spec.tsx ✅ Passed (Inspect) 3m 0s 0 0 0
formBuilder/inputs/PortableText/FocusTracking.spec.tsx ✅ Passed (Inspect) 45s 15 0 0
formBuilder/inputs/PortableText/Input.spec.tsx ✅ Passed (Inspect) 1m 43s 21 0 0
formBuilder/inputs/PortableText/ObjectBlock.spec.tsx ✅ Passed (Inspect) 1m 15s 18 0 0
formBuilder/inputs/PortableText/PresenceCursors.spec.tsx ✅ Passed (Inspect) 8s 3 9 0
formBuilder/inputs/PortableText/RangeDecoration.spec.tsx ✅ Passed (Inspect) 26s 9 0 0
formBuilder/inputs/PortableText/Styles.spec.tsx ✅ Passed (Inspect) 17s 6 0 0
formBuilder/inputs/PortableText/Toolbar.spec.tsx ✅ Passed (Inspect) 1m 14s 21 0 0
formBuilder/tree-editing/TreeEditing.spec.tsx ✅ Passed (Inspect) 1m 49s 30 0 0
formBuilder/tree-editing/TreeEditingNestedObjects.spec.tsx ✅ Passed (Inspect) 20s 3 0 0

@sjelfull sjelfull marked this pull request as ready for review August 1, 2024 11:41
@sjelfull sjelfull requested a review from a team as a code owner August 1, 2024 11:41
@sjelfull sjelfull requested review from juice49 and removed request for a team August 1, 2024 11:41
@sjelfull sjelfull marked this pull request as draft August 1, 2024 14:21
Signed-off-by: Fred Carlsen <fred@sjelfull.no>

Signed-off-by: Fred Carlsen <fred@sjelfull.no>
Signed-off-by: Fred Carlsen <fred@sjelfull.no>
Signed-off-by: Fred Carlsen <fred@sjelfull.no>
This makes sure we at least cover more of the studio side of the logic, and don’t rely solely on the unit test for the transferValue logic

Signed-off-by: Fred Carlsen <fred@sjelfull.no>
Signed-off-by: Fred Carlsen <fred@sjelfull.no>
The underlying issue is that the `getRecentSearches()` function will mutate state, and the state will not necessarily be updated before the test continues. The workaround here is to force a rerender any time we change the state before continuing. This might point to some underlying implementation of `recentSearches`, but I did not dig too much into that.

Signed-off-by: Fred Carlsen <fred@sjelfull.no>

Signed-off-by: Fred Carlsen <fred@sjelfull.no>
juice49
juice49 previously approved these changes Aug 13, 2024
Copy link
Contributor

@juice49 juice49 left a comment

Choose a reason for hiding this comment

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

This looks great!

Signed-off-by: Fred Carlsen <fred@sjelfull.no>
@sjelfull sjelfull dismissed stale reviews from RitaDias and juice49 via 577841c August 13, 2024 11:15
@sjelfull sjelfull added this pull request to the merge queue Aug 13, 2024
Merged via the queue into next with commit ea55826 Aug 13, 2024
25 of 30 checks passed
@sjelfull sjelfull deleted the fix/copy-paste-fixes branch August 13, 2024 11:19
cngonzalez pushed a commit that referenced this pull request Aug 20, 2024
…ray items (#7292)

* feat(core): support pasting object into array

touches edx-1432

Signed-off-by: Fred Carlsen <fred@sjelfull.no>

* fix(core): validate references in documents on paste

fixes edx-1584

Signed-off-by: Fred Carlsen <fred@sjelfull.no>

* fix(core): add missing translation key

Signed-off-by: Fred Carlsen <fred@sjelfull.no>

Signed-off-by: Fred Carlsen <fred@sjelfull.no>

* feat(core): add support for copying single array items

Signed-off-by: Fred Carlsen <fred@sjelfull.no>

* fix(core): handle first index segments (0) when getting value

Signed-off-by: Fred Carlsen <fred@sjelfull.no>

* test(core): add tests for copy paste hook

This makes sure we at least cover more of the studio side of the logic, and don’t rely solely on the unit test for the transferValue logic

Signed-off-by: Fred Carlsen <fred@sjelfull.no>

* chore(deps): add dependencies

Signed-off-by: Fred Carlsen <fred@sjelfull.no>

* fix(core): fix flaky test for recentSearches

The underlying issue is that the `getRecentSearches()` function will mutate state, and the state will not necessarily be updated before the test continues. The workaround here is to force a rerender any time we change the state before continuing. This might point to some underlying implementation of `recentSearches`, but I did not dig too much into that.

Signed-off-by: Fred Carlsen <fred@sjelfull.no>

Signed-off-by: Fred Carlsen <fred@sjelfull.no>

* chore(core): update comment about handling inline objects

Signed-off-by: Fred Carlsen <fred@sjelfull.no>

---------

Signed-off-by: Fred Carlsen <fred@sjelfull.no>
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.

3 participants