IA-4688: selected sampling results #2644
Conversation
quang-le
left a comment
quang-le
left a comment
There was a problem hiding this comment.
Minor things front-end side, but it seems to me the serializer needs to be reworked.
hat/assets/js/apps/Iaso/domains/plannings/hooks/useCanAssign.tsx
Outdated
Show resolved
Hide resolved
hat/assets/js/apps/Iaso/domains/plannings/components/PlanningForm.tsx
Outdated
Show resolved
Hide resolved
hat/assets/js/apps/Iaso/domains/plannings/components/SamplingResults.tsx
Outdated
Show resolved
Hide resolved
hat/assets/js/apps/Iaso/domains/plannings/hooks/requests/useDeletePlanning.ts
Outdated
Show resolved
Hide resolved
| selected_sampling_results_id = serializers.PrimaryKeyRelatedField( | ||
| queryset=PlanningSamplingResult.objects.all(), required=False, allow_null=True, write_only=True | ||
| ) | ||
| selected_sampling_results = NestedPlanningSamplingResultSerializer(read_only=True) |
There was a problem hiding this comment.
I don't understand why you need this. Also it seems weird to me to have a field for both the object and the id of the object
There was a problem hiding this comment.
Same thing for me, I don't understand why you have 2 attributes. Is it because you use the same serializer for read and write operations?
Co-authored-by: Quang Son Le <38907762+quang-le@users.noreply.github.com>
beygorghor
force-pushed
the
IA-4688-selected_sampling_results
branch
from
4b5d777 to
99d9305
Compare
iaso/models/microplanning.py
Outdated
| created_by = models.ForeignKey(User, on_delete=models.SET_NULL, null=True, blank=True) | ||
| created_at = models.DateTimeField(auto_now_add=True) | ||
| updated_at = models.DateTimeField(auto_now=True) | ||
| selected_sampling_results = models.ForeignKey( |
There was a problem hiding this comment.
I would use the singular form, since there's a FK to only one object
There was a problem hiding this comment.
Additional question: can a PlanningSamplingResult be selected by multiple plannings? It looks like it should be a 1to1 relation, but I'm not sure
| class PlanningWriteSerializer(PlanningSerializer): | ||
| def __init__(self, *args, **kwargs): | ||
| super().__init__(*args, **kwargs) | ||
| self.fields.pop("selected_sampling_results", None) |
There was a problem hiding this comment.
You do this because you want all fields, except selected_sampling_results?
If so, I would probably use except with __all__ instead, see DRF documentation
There was a problem hiding this comment.
The weird double attributes are probably due to the fact that this serializer inherits from the other one. I would make this one independant (inherit from models.ModelSerializer) and use except and __all__, it will probably simplify everything
| selected_sampling_results_id = serializers.PrimaryKeyRelatedField( | ||
| queryset=PlanningSamplingResult.objects.all(), required=False, allow_null=True, write_only=True | ||
| ) | ||
| selected_sampling_results = NestedPlanningSamplingResultSerializer(read_only=True) |
There was a problem hiding this comment.
Same thing for me, I don't understand why you have 2 attributes. Is it because you use the same serializer for read and write operations?
| def to_representation(self, instance): | ||
| data = super().to_representation(instance) | ||
| sampling = instance.selected_sampling_results | ||
| data["selected_sampling_results"] = NestedPlanningSamplingResultSerializer(sampling).data if sampling else None | ||
| return data |
There was a problem hiding this comment.
I'm not sure why you need this either
| def test_selected_sampling_results_field(self): | ||
| """Selected sampling result can be set and read back on planning.""" | ||
| sampling = PlanningSamplingResult.objects.create( | ||
| planning=self.planning, | ||
| pipeline_id="pipeline-123", | ||
| pipeline_version="v1", | ||
| pipeline_name="sample-run", | ||
| parameters={"foo": "bar"}, | ||
| ) | ||
|
|
||
| self.planning.selected_sampling_results = sampling | ||
| self.planning.save() | ||
| self.planning.refresh_from_db() | ||
|
|
||
| self.assertEqual(self.planning.selected_sampling_results, sampling) |
There was a problem hiding this comment.
That's not really necessary since there's no custom logic, this test is just testing Django's ORM. We can keep it but it doesn't really bring any value
tdethier
left a comment
tdethier
left a comment
There was a problem hiding this comment.
Much better now!
There are still a few things that should be done to make sure that the validation is correct (see TODOs in the tests). We'll do that in another PR
User should be able to select a sampling result that will be use for assignments step
Related JIRA tickets : IA-4688
Self proofreading checklist
Doc
Changes
selected_sampling_resultsfield to planning model and api + testsHow to test
Make sure you have sampling generated with samplings, you should be able to select one of those to be use with selected planning
Print screen / video
Follow the Conventional Commits specification
The merge message of a pull request must follow the Conventional Commits specification.
This convention helps to automatically generate release notes.
Use lowercase for consistency.
Example:
Note that the Jira reference is preceded by a line break.
Both the line break and the Jira reference are entered in the Add an optional extended description… field.