Fixes the bug related to the geometries of duplicated interventions - #5645
Open
PartyNell wants to merge 4 commits into
Open
Fixes the bug related to the geometries of duplicated interventions#5645PartyNell wants to merge 4 commits into
PartyNell wants to merge 4 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #5645 +/- ##
=======================================
Coverage 98.54% 98.54%
=======================================
Files 274 274
Lines 23025 23034 +9
=======================================
+ Hits 22690 22699 +9
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/5645/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 ↗︎ | |
|
|
||
| **Bug fixes** | ||
|
|
||
| * Fix geometry of duplicated intervention |
Contributor
There was a problem hiding this comment.
Suggested change
| * Fix geometry of duplicated intervention | |
| * Fix intervention duplication to also duplicate its target when linked to its own topology instead of another object (e.g. signage, etc) (refs #3845) |
| interv = InterventionFactory.create(target=None) | ||
| self.assertIn("-", interv.target_csv_display) | ||
|
|
||
| def test_duplication_with_topology_target(self): |
Contributor
There was a problem hiding this comment.
J'ajouterais bien une docstring pour expliquer ce qu'on teste, avec pourquoi pas un lien vers l'issue
| def duplicate(self, **kwargs): | ||
| clone = super().duplicate(**kwargs) | ||
| if clone.target_type == ContentType.objects.get_for_model(Topology): | ||
| # if the target is a topology create a new one with the same information |
Contributor
There was a problem hiding this comment.
Suggested change
| # if the target is a topology create a new one with the same information | |
| # if the target is a topology instead of another object (e.g. signage, etc), create a new one with the same information |
| interv = InterventionFactory.create(target=None) | ||
| self.assertIn("-", interv.target_csv_display) | ||
|
|
||
| def test_duplication_with_topology_target(self): |
Contributor
There was a problem hiding this comment.
Ajouter un test pour vérifier que dans le cas où l'intervention est liée à un autre objet, cet objet n'est pas dupliqué / une nouvelle topologie n'est pas créée (en gros il faut tester le cas où on ne rentre pas dans le if)
| topology_copy = intervention_copy.target | ||
| self.assertEqual(topology_copy.geom, topology.geom) | ||
| self.assertEqual(topology_copy.offset, topology.offset) | ||
| self.assertEqual(topology_copy.kind, "INTERVENTION") |
Contributor
There was a problem hiding this comment.
Vérifier que topology_copy et topology n'aient pas le même pk
Comment on lines
+353
to
+355
| Intervention.duplicate(intervention) | ||
| self.assertEqual(Intervention.objects.count(), 2) | ||
| intervention_copy = Intervention.objects.get(name="intervention test (copy)") |
Contributor
There was a problem hiding this comment.
Suggested change
| Intervention.duplicate(intervention) | |
| self.assertEqual(Intervention.objects.count(), 2) | |
| intervention_copy = Intervention.objects.get(name="intervention test (copy)") | |
| intervention_copy = Intervention.duplicate(intervention) | |
| self.assertEqual(Intervention.objects.count(), 2) |
2 avantages :
- on teste la valeur de retour de la méthode
- fonctionnera toujours si on change le nom généré par défaut lors d'une duplication (pas géré par la méthode qu'on teste ici)
Comment on lines
171
to
180
| fields_name_different = [ | ||
| "id", | ||
| "uuid", | ||
| "date_insert", | ||
| "date_update", | ||
| "name", | ||
| "name_en", | ||
| "target_id", | ||
| "target", | ||
| ] |
Contributor
There was a problem hiding this comment.
C'était comme ça de base, mais en profiter pour sortir cette définition liste de dans la boucle for, et la mettre juste au dessus ? pas besoin de la redéfinir à chaque tour de boucle
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
When duplicating an intervention based on a path, the topology representing the geometry was not duplicated. As a result, both interventions shared the same geometry.
Related Issue
Checklist
AI requirements
Skip the checkboxes below 👇 If you didn't use AI for your contribution