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

fixed issue that didn't allow a user to update a document when deleting elements in an array attribute #495

Merged
merged 3 commits into from
Aug 11, 2023
Merged

fixed issue that didn't allow a user to update a document when deleting elements in an array attribute #495

merged 3 commits into from
Aug 11, 2023

Conversation

safwanyp
Copy link
Contributor

@safwanyp safwanyp commented Aug 8, 2023

Use symmetricDifference instead of difference.

  • difference only returns values that are present in the edited attributes, but not in the originally fetched attribute vales. This always ends up returning an empty array.
  • I used symmetricDifference as it returns an intersection of difference from both sides and works perfectly for this use case, as the original attributes never change.

What does this PR do?

This PR closes appwrite/appwrite#5476

Test Plan

  • In addition to the existing tests, I performed manual testing by adding and deleting elements in an array attribute.
  • I was going to change the way the difference function worked, but realized it was being used in other places, which broke things. I used the symmetricDifference function as it fits in perfectly for this use case.

Related PRs and Issues

Have you read the Contributing Guidelines on issues?

Yes ✅

Use `symmetricDifference` instead of `difference`.
- `difference` only returns values that are present in the edited attributes, but not in the originally fetched attribute vales. This always ends up returning an empty array.
- I used `symmetricDifference` as it returns an intersection of `difference` from both sides and works perfectly for this use case, as the original attributes never change.
@vercel
Copy link

vercel bot commented Aug 8, 2023

@safwanyp is attempting to deploy a commit to the appwrite Team on Vercel.

A member of the Team first needs to authorize it.

@safwanyp
Copy link
Contributor Author

safwanyp commented Aug 8, 2023

Okay I just realized there's another bug.
Although elements can be deleted, existing items cannot be edited. The Update button stays disabled even after changing values. Should I create a new issue for that, or push a commit to fix it in this PR?

@safwanyp safwanyp changed the title fixed incorrect array being used in comparison fixed issue that didn't allow a user to update a document when deleting elements in an array attribute Aug 8, 2023
@safwanyp
Copy link
Contributor Author

safwanyp commented Aug 9, 2023

Okay so I figured out the issue that prevents updating of a document when editing the elements of an array attribute.

In the code it seems like $work stores the value of edited values. $work is populated by iterating over $doc, but they are referencing the same underlying value, no? Due to this, when $work is populated, $doc is updated as well, which results in both the arrays having no difference.

I can think of 2 solutions to this:

  1. Use JSON.parse(JSON.stringify($doc)) to clone the $doc, and not reference copy.
  2. Use a locally decalred variable instead of formValues[attribute.key][index] in <AttributeItem />, which can be passed into the <Attribute /> component. Obviously, the local variable will be updated when there's a change.

I have only tested with the first approach as it doesn't mess with the functionality outside of the required component.

@TGlide TGlide requested review from ArmanNik and TGlide August 9, 2023 11:51
@TGlide
Copy link
Contributor

TGlide commented Aug 9, 2023

Okay so I figured out the issue that prevents updating of a document when editing the elements of an array attribute.

In the code it seems like $work stores the value of edited values. $work is populated by iterating over $doc, but they are referencing the same underlying value, no? Due to this, when $work is populated, $doc is updated as well, which results in both the arrays having no difference.

I can think of 2 solutions to this:

1. Use `JSON.parse(JSON.stringify($doc))` to clone the $doc, and not reference copy.

2. Use a locally decalred variable instead of `formValues[attribute.key][index]` in `<AttributeItem />`, which can be passed into the `<Attribute />` component. Obviously, the local variable will be updated when there's a change.

I have only tested with the first approach as it doesn't mess with the functionality outside of the required component.

Hi @safwanyp! Thank you for the PR 😄

I'd say doing a deepClone with JSON.parse should be enough. I'd recommend creating an utility for it in helpers/object.ts

- Original code creates a reference copy of `$doc` into `$work` which means that editing values of `$work` changes the values in `$doc`.
- Fix this behaviour by creating a deep clone of `$doc` into `$work`, so underlying values aren't cross-referenced.
@safwanyp
Copy link
Contributor Author

Hey @TGlide, thanks for the feedback and pointers!

I've just pushed a commit that creates a deepClone utility function, and uses it to create a copy of $doc into $work. Tests were successful on local copy. Appreciate any feedback!

Comment on lines 26 to 42
deepClone(
Object.keys($doc)
.filter((key) => {
return ![
'$id',
'$collection',
'$collectionId',
'$databaseId',
'$createdAt',
'$updatedAt'
].includes(key);
})
.reduce((obj, key) => {
obj[key] = $doc[key];
return obj;
}, {}) as Models.Document
)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this function has become quite verbose.

As an alternative, we can create an initWork function, where we have better control flow.

e.g.

function initWork() {
  let prohibitedKeys = ['$id', ...];
  const filteredKeys = Object.keys($doc)
  // So on and so forth..
}

const work = initWork()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's fair. I'll work on this 👍🏻

Instead of cramming all the steps for correct deep cloning, I split the implementation into 4 distinct steps:
- define they keys to exclude
- filtering the keys
- reduce filtered keys into an object
- returning a writable deep clone
@safwanyp safwanyp requested a review from TGlide August 10, 2023 11:53
@vercel
Copy link

vercel bot commented Aug 11, 2023

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

Name Status Preview Comments Updated (UTC)
console ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 11, 2023 1:04pm
console-cloud ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 11, 2023 1:04pm
console-next ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 11, 2023 1:04pm

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.

🐛 Bug Report: Can't update document after deleting array element
3 participants