-
Notifications
You must be signed in to change notification settings - Fork 31
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: Add cleanRelatedContacts service #1019
Conversation
BundleMonFiles added (1)
Files updated (3)
Unchanged files (4)
Total files change +304.98KB +16.23% Groups updated (1)
Unchanged groups (2)
Final result: ✅ View report in BundleMon website ➡️ |
e32419e
to
02eb124
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe another review from @JF-Cozy or @paultranvan because I'm not familiar with services and it's about deletion so sensitive ?
But looks good from my view.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
on manipule de la data ici, c'est typiquement le cas à tester correctement (sujet du /alpha
) perso je mettrai ça derrière un flag
import { DOCTYPE_CONTACTS } from '../helpers/doctypes' | ||
|
||
export const cleanRelatedContactsService = async (client, contactDeletedId) => { | ||
const allContacts = await client.queryAll(Q(DOCTYPE_CONTACTS)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
petite interrogation sur await client.queryAll(Q(DOCTYPE_CONTACTS))
est-ce une bonne manière de faire ? @paultranvan
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
En m'inspirant d'une query existante, ceci serait sans doute plus pertinent.
Q(DOCTYPE_CONTACTS)
.where({
relationships: {
related: {
data: {
$elemMatch: {
_id: contactDeletedId,
_type: DOCTYPE_CONTACTS
}
}
}
}
})
.indexFields(['relationships.related.data'])
.limitBy(1000)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
est-ce une bonne manière de faire ?
On est dans un service, donc moins contraint par la pagination que sur une app. Ceci étant dit, ça ne nous absoud pas de ne requêter que ce qui est nécessaire.
Cf #1019 (comment) pour la requête
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Vu ensemble, nous allons terminer prochainement cette fonctionnalité de relation entre contact, qui devrait simplifier le besoin ici.
Le temps de, je vais juste ajouter un commentaire pour ne pas oublier de repasser sur cette query.
eb657de
to
b80f91f
Compare
data: { | ||
$elemMatch: { | ||
_id: contactDeletedId, | ||
_type: DOCTYPE_CONTACTS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This elemMatch
operator is to avoid when possible, because it will force an index full scan.
But why not simply use the include
dsl ?
Q(DOCTYPE_CONTACTS).include('related')
``
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On afterthought, using this query will also make a full-scan, and then query all the the relationships, so not good either...
When you delete a contact, this service deletes any relationships it may have had with other contacts.
b80f91f
to
9e69ffe
Compare
When you delete a contact, this service deletes any relationships it may have had with other contacts.