-
Notifications
You must be signed in to change notification settings - Fork 9
IA-4626: new Lqas pipeline version #2574
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
Conversation
tdethier
left a comment
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.
The feature works fine 👍
I'm requesting changes because I think that we are missing some checks in geo restrictions
iaso/api/groups/serializers.py
Outdated
| org_units = validated_data.pop("org_units", []) | ||
| default_version = self._fetch_user_default_source_version() | ||
| validated_data["source_version"] = default_version | ||
| return super().create(validated_data) | ||
| group = super().create(validated_data) | ||
| if org_units: | ||
| group.org_units.set(org_units) |
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.
I thought that org_units was the field used for read operations and org_unit_ids for write operations. Why do you use org_units in create()?
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.
✅
iaso/tests/api/groups/test_views.py
Outdated
| marvel = self.raccoon.iaso_profile.account | ||
| marvel_data_source = m.DataSource.objects.create(name="Marvel source") | ||
| marvel_version = m.SourceVersion.objects.create(data_source=marvel_data_source, number=1) | ||
| marvel_org_unit_type = m.OrgUnitType.objects.create(name="Marvel HC", short_name="MHC") | ||
| marvel_org_unit = m.OrgUnit.objects.create( | ||
| name="Marvel Org Unit", version=marvel_version, org_unit_type=marvel_org_unit_type |
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.
Someone is not going to be happy with these names 😁
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.
i followed the mood of the file sorry
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.
That would be me. I'm sorry, but I'm blocking over this. Please rename using expressive names like user_from_different_account. It really makes understanding, debugging and refactoring easier for everyone
iaso/tests/api/groups/test_views.py
Outdated
| def test_groups_create_with_org_units_user_with_no_restrictions(self): | ||
| """POST /groups/ user with no editable_org_unit_types restrictions can add any org unit""" | ||
|
|
||
| special_org_unit_type = m.OrgUnitType.objects.create(name="Special Type", short_name="ST") | ||
| special_org_unit = m.OrgUnit.objects.create( | ||
| name="Special Org Unit", version=self.source_version_2, org_unit_type=special_org_unit_type | ||
| ) | ||
|
|
||
| self.client.force_authenticate(self.yoda) | ||
| response = self.client.post( | ||
| "/api/groups/", | ||
| data={"name": "test group", "org_unit_ids": [special_org_unit.id, self.org_unit_1.id]}, | ||
| format="json", | ||
| ) | ||
| self.assertJSONResponse(response, 201) | ||
|
|
||
| group = m.Group.objects.get(id=response.json()["id"]) | ||
| self.assertEqual(group.org_units.count(), 2) |
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.
That's already what you check in the happy path, I'd say we can get rid of this test
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.
✅
| editable_org_unit_type_ids = profile.get_editable_org_unit_type_ids() | ||
| if editable_org_unit_type_ids: | ||
| queryset = queryset.filter(org_unit_type_id__in=editable_org_unit_type_ids) |
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.
That's one half of the geo restrictions covered, great!
A user can also be restricted to some branches of a pyramid (org_units in Profile) , so I guess we need to make sure that the received IDs belong to the pyramid section of that user 😅
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.
✅
quang-le
left a comment
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.
Just code review without feature test at this stage.
Nice job with spotting the buggy behaviour with staleTime
| selectedItem, | ||
| search, | ||
| }: Props): Result => { | ||
| const [extraFilters, setExtraFilters] = useState<Record<string, any>>({}); |
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.
if it goes in the params isn't it Record<string,string>?
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.
✅
| setSelectedPipelineId(value); | ||
| setParameterValues(undefined); | ||
| setAllowConfirm(false); | ||
| }, []); |
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.
Don't use useCallbackwith empty deps array 😠
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.
✅
| useEffect(() => { | ||
| if (taskDetails?.result?.group_id) { | ||
| setExtraFilters({ | ||
| group: taskDetails.result.group_id, | ||
| }); | ||
| } |
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.
Won't this override any existing extraFilters?
Reminder: there's a useObjectState utils that allows us to use useState syntax with objects similar to the setState of class components
Also, instead of prop drilling setExtraFilters you could pass it through a Context, but maybe there's a perf price to pay
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.
I would leave it like that, next step is to refactor this part of the code
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.
OK then
| useEffect(() => { | ||
| if ( | ||
| parameterValues?.org_unit_type_sequence_identifiers?.length && | ||
| parameterValues?.org_unit_type_sequence_identifiers?.length > 0 && | ||
| !canAddLevel | ||
| org_unit_type_sequence_identifiers?.length && | ||
| org_unit_type_sequence_identifiers?.length > 0 | ||
| ) { | ||
| const allLevelsFilled = | ||
| parameterValues.org_unit_type_sequence_identifiers?.every( | ||
| (level, index) => { | ||
| return ( | ||
| level !== undefined && | ||
| parameterValues.org_unit_type_criteria?.[index] !== | ||
| undefined && | ||
| parameterValues.org_unit_type_quantities?.[ | ||
| index | ||
| ] !== undefined | ||
| ); | ||
| }, | ||
| ); | ||
| setAllowConfirm(Boolean(allLevelsFilled)); | ||
| const allLevelsFilled = org_unit_type_sequence_identifiers?.every( | ||
| (level, index) => { | ||
| return ( | ||
| level !== undefined && | ||
| org_unit_type_criteria?.[index] !== undefined && | ||
| org_unit_type_quantities?.[index] !== undefined | ||
| ); | ||
| }, | ||
| ); | ||
|
|
||
| setAllowConfirm( | ||
| Boolean(allLevelsFilled) && | ||
| (!planning.target_org_unit_type || | ||
| latestOptions?.value === planning.target_org_unit_type), | ||
| ); | ||
| } else { | ||
| setAllowConfirm(false); | ||
| } | ||
| }, [setAllowConfirm, parameterValues, canAddLevel]); |
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.
The goal of this useEffect seems to be to set the value of 'allowConfirm. If so, it would be better to do it via useMemo`
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.
i don't get it, the setAllowConfirm is level above
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.
OK I get it now
| created_at = TimestampField(read_only=True) | ||
| updated_at = TimestampField(read_only=True) |
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.
Don't we usually use DateField or DateTimeField for the web ?
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 is used a bit everywhere
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.
Yeah, but didn't we want to move away from Timestamps in the front-end?
iaso/tests/api/groups/test_views.py
Outdated
| marvel = self.raccoon.iaso_profile.account | ||
| marvel_data_source = m.DataSource.objects.create(name="Marvel source") | ||
| marvel_version = m.SourceVersion.objects.create(data_source=marvel_data_source, number=1) | ||
| marvel_org_unit_type = m.OrgUnitType.objects.create(name="Marvel HC", short_name="MHC") | ||
| marvel_org_unit = m.OrgUnit.objects.create( | ||
| name="Marvel Org Unit", version=marvel_version, org_unit_type=marvel_org_unit_type |
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.
That would be me. I'm sorry, but I'm blocking over this. Please rename using expressive names like user_from_different_account. It really makes understanding, debugging and refactoring easier for everyone
tdethier
left a comment
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.
LGTM, thanks for making the changes 👍
Adapt IASO to fit to the latest LQAS pipeline on Openhexa
Related JIRA tickets : IA-4626
Self proofreading checklist
Doc
Changes
How to test
Print screen / video
Screen.Recording.2025-11-28.at.11.34.26.mov
Notes
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.