Fix site duplication so that the clone ID is used as the tree ID if the original site doesn't have a parent - #5646
Fix site duplication so that the clone ID is used as the tree ID if the original site doesn't have a parent#5646PartyNell wants to merge 1 commit into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #5646 +/- ##
=======================================
Coverage 98.54% 98.54%
=======================================
Files 274 274
Lines 23034 23040 +6
=======================================
+ Hits 22699 22705 +6
Misses 335 335 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Geotrek-admin
 |
||||||||||||||||||||||||||||
| Project |
Geotrek-admin
|
| Branch Review |
refs/pull/5646/merge
|
| Run status |
|
| Run duration | 02m 09s |
| Commit |
|
| Committer | Nell Party |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
 |
0
|
 |
0
|
 |
0
|
 |
0
|
 |
22
|
| View all changes introduced in this branch ↗︎ | |
| https://github.com/GeotrekCE/Geotrek-admin/issues/5461 | ||
| """ | ||
| clone = super().duplicate(**kwargs) | ||
| if clone.parent is None: |
There was a problem hiding this comment.
J'utiliserais la méthode dédiée : https://django-mptt.readthedocs.io/en/latest/models.html#is-root-node
| clone.tree_id = clone.id | ||
| clone.save(update_fields=["tree_id"]) |
There was a problem hiding this comment.
Je trouve cette solution risquée pour plusieurs raisons :
- que se passe-t-il si la valeur de
clone.idest déjà attribuée à un arbre ? je ne sais pas comment sont gérées les valeurs detree_idmais imaginons qu'elles fonctionnent comme une séquence en sql. si on supprime des arbres puis on en recrée et modifiant les liens entre sites, on peut se retrouver avec destree_idsupérieurs ou égaux auxpkdes sites. on ne sait jamais ce qui peut se passer et comment le code (de gta ou de la lib) peut évoluer - on attribue à la main une valeur qui fait partie de tout un système géré sous le capot dans une lib. il y a le risque de, par exemple, passer à côté d'une autre valeur à modifier, et si ce n'est pas le cas actuellement, la lib peut évoluer -> il faudra non seulement ne pas passer à côté de l'évolution, mais en plus faire une PR dédiée pour être compatible avec la nouvelle version.
Je n'ai pas testé, mais utiliser la méthode move_to avec target à None me semble être une solution plus safe car tout est géré par la lib :
https://django-mptt.readthedocs.io/en/latest/models.html#move-to-target-position-first-child
Qu'en penses-tu ?
There was a problem hiding this comment.
Après avoir testé move_to, il ne fonctionne pas dans ce cas.
There was a problem hiding this comment.
Le soucis c'est que move_to déplace un objet en racine uniquement s'il n'est pas déjà en racine. Hors dans notre cas c'est pas le cas.
There was a problem hiding this comment.
Voilà la solution que j'ai trouvé mais c'est pas génial étant donnée qu'on utilise des méthodes qui ne sont pas censées être utilisées hors de la classe.
def duplicate(self, **kwargs):
"""
Duplicate the site and ensure that a duplicated root site gets its own tree ID.
https://github.com/GeotrekCE/Geotrek-admin/issues/5461
"""
clone = super().duplicate(**kwargs)
if clone.is_root_node():
manager = clone._tree_manager
new_tree_id = manager._get_next_tree_id()
clone.tree_id = new_tree_id
clone.save(update_fields=["tree_id"])
return clone
There was a problem hiding this comment.
Sinon les autres solutions sont :
- bouger artificiellement clone sur un autre arbre en tant que enfant puis faire un move
- utiliser Site.objects.rebuild() qui reprend toutes les root et leur attribut un tree_id mais c'est coûteux en calcul
| self.site_2 = SiteFactory.create(name="child_site", parent=self.site_1) | ||
| clone = self.site_1.duplicate() | ||
| self.assertNotEqual(clone.tree_id, self.site_1.tree_id) | ||
| self.assertEqual(list(self.site_2.get_ancestors()), [self.site_1]) |
There was a problem hiding this comment.
Vérifier plutôt/en plus que la valeur de retour de clone.get_family() ne contienne qu'un seul élément (clone lui-même) ?
On s'assure alors qu'on a bien créé un tout nouvel arbre : le clone n'a ni parent ni enfant.
Selon moi c'est plus complet et plus direct (donc plus compréhensible pour le lecteur) que vérifier les ancêtres de site_2.
https://django-mptt.readthedocs.io/en/latest/models.html#get-family
2 participants
Description
If the duplicate site does not have a parent, set its tree ID to its ID. Otherwise, the children of the initial site will be unable to retrieve the root of the hierarchy because there are multiple roots (the root of a hierarchy is defined by the tree ID).
Related Issue
Checklist
AI requirements
Skip the checkboxes below 👇 If you didn't use AI for your contribution