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

[SCR-164] feat: New strategy for ensureKonnectorFolder #1443

Merged
merged 3 commits into from
Feb 19, 2024

Conversation

doubleface
Copy link
Contributor

Now konnector destination folders will have the following references:

  • Main konnector folder : with a reference to the associated konnector
    slug
  • konnector account folder : with a reference to the associated
    konnector slug + another reference to the associated
    sourceAccountIdentifier

This way, even if the user moves/or rename the konnector folder or the
konnector account folder, the konnector will find it and reuse it after
konnector disconnect/reconnect.

Existing konnector main folders and konnector account folders will be
migrated to these references

  • docs: Add missing dir_id to IOCozyFolder type

@doubleface doubleface force-pushed the feat/ensureKonnectorFolderWithReferences branch 4 times, most recently from 9378fbd to 0bc07eb Compare February 7, 2024 15:47
@doubleface doubleface marked this pull request as ready for review February 7, 2024 15:47
@doubleface
Copy link
Contributor Author

@Crash-- @paultranvan This PR is ready to review

!isTrashedFolder(file) &&
hasSourceAccountIdentifierReference(file, sourceAccountIdentifier)
)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

You are bypassing all the cozy-client built-in query features, which is not good 😉
First, why using collection and not cozy-client query ? e.g. client.query(Q('io.cozy.files').referencedBy({...})
This way, you will benefit from the store

Second, you query the references, and after that filter the retrieved values, while it is precisely the client.query job.
So, that would make something like this:

const resp = await client.query(Q('io.cozy.files').partialIndex({type: "directory", trashed: false}.referencedBy({...})

For the hasSourceAccountIdentifierReference, I'm not sure I understand correctly, the file must have a io.cozy.konnectors reference + io.cozy.accounts.sourceAccountIdentifier? And I'm surprised about this last one, is it a new doctype?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did not know about this Q().referencedBy, I just took the same method used to query magic folders in folder.js. Ok, let's use cozy-client query (And I will update folder.js.

And second, I'm surprised the request you propose works, since the collection findReferencedBy uses a specific stack request but I see there are helpers for this in the QueryDefinition. let's do this then, there will be a lot less code 👍.

packages/cozy-client/src/models/konnectorFolder.js Outdated Show resolved Hide resolved
@doubleface doubleface force-pushed the feat/ensureKonnectorFolderWithReferences branch from 0bc07eb to 4db3e0c Compare February 13, 2024 16:38
Now konnector destination folders will have the following references:

- Main konnector folder : with a reference to the associated konnector
  slug
- konnector account folder : with a reference to the associated
  konnector slug + another reference to the associated
  sourceAccountIdentifier

This way, even if the user moves/or rename the konnector folder or the
konnector account folder, the konnector will find it and reuse it after
konnector disconnect/reconnect.

Existing konnector main folders and konnector account folders will be
migrated to these references
@doubleface doubleface force-pushed the feat/ensureKonnectorFolderWithReferences branch from 4db3e0c to c7fe23a Compare February 13, 2024 17:02
@doubleface doubleface force-pushed the feat/ensureKonnectorFolderWithReferences branch from c7fe23a to 7c6edf6 Compare February 13, 2024 17:15
Copy link
Contributor

@paultranvan paultranvan left a comment

Choose a reason for hiding this comment

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

I didn't have time to completely understand this PR scope, and my main concerns looks addressed, so nothing blocking on my side

@doubleface
Copy link
Contributor Author

@Crash-- Let's merge this on next monday morning if I have no news.

@doubleface doubleface merged commit af2bb58 into master Feb 19, 2024
4 checks passed
@doubleface doubleface deleted the feat/ensureKonnectorFolderWithReferences branch February 19, 2024 08:04
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.

2 participants