Get permission service: clean "can_request_help" usage #1006

GeoffreyHuck · 2023-09-08T14:22:42Z

We now return an array of all groups with can_request_help_to permissions set in all granted_via_*.

In computed, we also return all permissions coming from parent items while request_help_propagation is set. The uses a recursive query that is only called once, GetAncestorsRequestHelpPropagatedQuery().

Format of `can_request_help_to` in response

id always set
name present only if it is visible
is_all_users_group to true or false depending on the case

Performance notice:

We use the query IsVisibleForGroup() once for each group we find. So it might be expensive if we have cases where we have many groups can_request_help_to permissions set.

The typical query is the following:

SELECT 1 FROM `groups` 
WHERE (groups.is_public OR groups.id IN(
    SELECT groups_ancestors_active.ancestor_group_id
    FROM `groups_groups_active`
    JOIN groups_ancestors_active ON groups_ancestors_active.child_group_id = groups_groups_active.parent_group_id
    JOIN `groups` AS ancestor_group ON ancestor_group.id = groups_ancestors_active.ancestor_group_id
    WHERE (groups_groups_active.child_group_id = 5549855599337303585)
    AND (ancestor_group.type != 'ContestParticipants'
))
OR groups.id IN(
    SELECT ancestors_of_managed.ancestor_group_id FROM `groups_ancestors_active` 
    JOIN group_managers ON group_managers.group_id = `groups_ancestors_active`.ancestor_group_id 
    JOIN groups_ancestors_active AS group_ancestors ON group_ancestors.ancestor_group_id = group_managers.manager_id
        AND group_ancestors.child_group_id = 5549855599337303585
    JOIN `groups` ON groups.id = groups_ancestors_active.child_group_id 
    JOIN groups_ancestors_active AS ancestors_of_managed ON ancestors_of_managed.child_group_id = groups_ancestors_active.child_group_id
        AND (groups.type != 'User' OR ancestors_of_managed.is_self)
    JOIN `groups` AS ancestor_group ON ancestor_group.id = ancestors_of_managed.ancestor_group_id
    WHERE (ancestor_group.type != 'ContestParticipants')
))
AND (groups.id = 2646504854435585587)
LIMIT 1

Note: It might be easier to review commit by commit. Notes about choices are present in commit messages.

… granted_via_* For now, it is always an empty array

We need it to test the viewPermissions service

…abase if it doesn't exists It seems obvious but it wasn't the case because database foreign keys are disabled in tests

Because we have cases that weren't covered to test for the viewPermissions service changes Note: we don't distinguish between empty string and nil. It doesn't seem important now, but it might be a requirement in the future.

…s for group_membership, item_unlocking, self and other Also changed the structure of a can_request_help_to permission: id, is_all_users_group and name are now always present. It improves consistency. Not implemented for computed yet @see #1004

…_request_help_to Note: it doesn't contain the groups who don't appear in the other groups yet

Also, return groups with a different group_source_id in granted_via_*, it was forgotten

…opagation So we can test viewPermission with can_request_help_to in the computed group with propagation

… when the permission propagates

… origins. It simplifies the query and the code

…xity

codecov · 2023-09-11T07:22:05Z

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (cb9e4c9) 100.00% compared to head (dd8741a) 100.00%.
Report is 8 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff            @@
##            master     #1006   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          230       230           
  Lines        13847     13893   +46     
=========================================
+ Hits         13847     13893   +46

Files Changed	Coverage Δ
app/api/groups/get_permissions.go	`100.00% <100.00%> (ø)`
app/database/item_store.go	`100.00% <100.00%> (ø)`
app/database/user.go	`100.00% <100.00%> (ø)`

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

smadbe

You write in the PR:

Format of can_request_help_to in response

The fields are now always present:

id always set

name set or null if not visible

is_all_users_group to true or false depending on the case

At other places of the API, when an attribute is not visible, it is omitted (not given)... null is reserved for cases when a value is visible but null (the db value).

app/api/groups/get_permissions_can_request_help_to.feature

smadbe · 2023-09-11T14:50:21Z

app/api/groups/get_permissions_can_request_help_to.feature

-    And the response at $.granted.can_request_help_to should be "null"
+    And the response at $.granted.can_request_help_to should be:
+      | id           | name | is_all_users_group |
+      | @HelperGroup |      | false              |


Again, this makes it difficult for me to check if this return a tuple (as requested) or just an array with one tuple (which would be more consistent with the other tables).

fixed with <undefined>, see comment below

I think you didn't get my point. My question was : how can I know reading this if you checking for a tuple<id, name, is_all_users_group> or an array containing one tuple<id, name, is_all_users_group> ? What would be the different in the rule?

The difference is in the JSONPath. To retrieve an array, there is a [*] at the end of the JSONPath:

$.granted.can_request_help_to is an object

$.granted_via_membership.can_request_help_to[*] is an array

EDIT: I just checked and it works without the [*]. So

Either we intentionally put the [*] when we want an array

Or, we create a different feature for arrays. The code is basically doing a if (is_array) else {} already, so this might be a better solution

EDIT 2: Implemented as JSON checking

app/api/groups/get_permissions_can_request_help_to.feature

So we can differentiate between null, empty arrays and empty strings

GeoffreyHuck · 2023-09-12T13:04:15Z

The ways to check for null, empty arrays, undefined fields, and empty strings have been uniformed:

<null> for null
[] for empty array
<undefined> for when the field is not present
`` nothing for an empty string

The feature the response should not be defined at JSONPath have been deleted in favor of the response at JsonPath should be "<undefined>" so we make the checks in the same way whether we check for a value or in an array with a table:

the response at $.groups[*] should be:
| id | name        |
| 1  | AAA         |
| 2  | <undefined> |

For the same reason, the empty array is checked with a special value instead of another feature, so we can check for empty arrays in an array.

To comply with the architecture decision We use <undefined> to represent a field that is not present, so we can distinguish it with null

…ature To uniformize the way we check for undefined path in arrays

smadbe · 2023-09-12T14:57:53Z

app/api/groups/get_permissions_can_request_help_to.feature

+    And the response at $.granted_via_self.can_request_help_to[*] should be "[]"
+    And the response at $.granted.can_request_help_to should be "<null>"
+    And the response at $.granted_via_group_membership.can_request_help_to[*] should be "[]"
+    And the response at $.granted_via_item_unlocking.can_request_help_to[*] should be "[]"


Why not $.granted_via_item_unlocking.can_request_help_to should be "[]" ?

smadbe · 2023-09-12T15:01:16Z

app/api/groups/get_permissions_can_request_help_to.feature

-    And the response at $.granted.can_request_help_to should be "null"
+    And the response at $.granted.can_request_help_to should be:
+      | id           | name | is_all_users_group |
+      | @HelperGroup |      | false              |


I think you didn't get my point. My question was : how can I know reading this if you checking for a tuple<id, name, is_all_users_group> or an array containing one tuple<id, name, is_all_users_group> ? What would be the different in the rule?

smadbe

Mainly questions actually...

This makes it clearer when we test single objects, rather than using an array

It's clearer that we are testing a single object and not an array with one entry @see #1006 (comment)

…t we are testing an array The check for single object with this feature has been disabled, it should now be tested with featuer allowing to check JSON at a JSONPath

GeoffreyHuck added 12 commits September 8, 2023 13:15

ViewPermissions: add can_request_help_to array fields to computed and…

c5fced9

… granted_via_* For now, it is always an empty array

Gherkin item permissions: Support source_group_id and origin

38c6426

We need it to test the viewPermissions service

Gherkin item permissions: Create the group can_request_help_to in dat…

6efa6b3

…abase if it doesn't exists It seems obvious but it wasn't the case because database foreign keys are disabled in tests

Gherkin comparison: Handle nil values

dfe77ed

Because we have cases that weren't covered to test for the viewPermissions service changes Note: we don't distinguish between empty string and nil. It doesn't seem important now, but it might be a requirement in the future.

viewPermission: computed now contain the aggregation of the other can…

33c6e60

…_request_help_to Note: it doesn't contain the groups who don't appear in the other groups yet

viewPermissions: add computed can_request_help_to groups

e476970

Also, return groups with a different group_source_id in granted_via_*, it was forgotten

Gherkin item relation: Add a definition to add item relations with pr…

2978654

…opagation So we can test viewPermission with can_request_help_to in the computed group with propagation

viewPermission: Add can_request_help_to from parent items in computed…

9389a66

… when the permission propagates

viewPermission: Refactor get can_request_help_to group with the other…

1deed0a

… origins. It simplifies the query and the code

Refactor jsonPathResultMatchesValue because of lint cyclomatic comple…

17a052a

…xity

viewPermission: Refactor the computation of can_request_help_to groups

b35581b

GeoffreyHuck changed the title ~~[WIP] Get permission service: clean "can_request_help" usag~~ [WIP] Get permission service: clean "can_request_help" usage Sep 11, 2023

GeoffreyHuck changed the title ~~[WIP] Get permission service: clean "can_request_help" usage~~ Get permission service: clean "can_request_help" usage Sep 11, 2023

GeoffreyHuck requested a review from smadbe September 11, 2023 07:46

smadbe requested changes Sep 11, 2023

View reviewed changes

GeoffreyHuck mentioned this pull request Sep 12, 2023

Add architecture decision about non-visible fields France-ioi/algorea-devdoc#41

Merged

Gherkin: Uniformize checks of JSONPath responses with <null>, []

2da8b63

So we can differentiate between null, empty arrays and empty strings

GeoffreyHuck added 2 commits September 12, 2023 15:50

viewPermission: remove can_request_help_to.name when it is not visible

3e48b77

To comply with the architecture decision We use <undefined> to represent a field that is not present, so we can distinguish it with null

Gherkin: test undefined paths with <undefined> instead of having a fe…

7eded6d

…ature To uniformize the way we check for undefined path in arrays

GeoffreyHuck requested a review from smadbe September 12, 2023 14:33

smadbe reviewed Sep 12, 2023

View reviewed changes

smadbe requested changes Sep 12, 2023

View reviewed changes

GeoffreyHuck added 2 commits September 13, 2023 09:46

Gherkin: Add feature to check the result of a JSONPath as JSON

79d5e77

This makes it clearer when we test single objects, rather than using an array

viewPermission: test $.granted.can_request_help_to with the JSON content

960d83b

It's clearer that we are testing a single object and not an array with one entry @see #1006 (comment)

GeoffreyHuck requested a review from smadbe September 13, 2023 08:01

Gherkin: The feature to check an array at a JSONPath now enforces tha…

dd8741a

…t we are testing an array The check for single object with this feature has been disabled, it should now be tested with featuer allowing to check JSON at a JSONPath

smadbe approved these changes Sep 13, 2023

View reviewed changes

GeoffreyHuck merged commit 73c262c into master Sep 13, 2023
4 checks passed

GeoffreyHuck deleted the 1004 branch September 13, 2023 09:12

zenovich mentioned this pull request Aug 4, 2024

Ineffective implementation of can_request_help in permissionsView #1141

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Get permission service: clean "can_request_help" usage #1006

Get permission service: clean "can_request_help" usage #1006

GeoffreyHuck commented Sep 8, 2023 •

edited

Loading

codecov bot commented Sep 11, 2023 •

edited

Loading

smadbe left a comment

smadbe Sep 11, 2023

GeoffreyHuck Sep 12, 2023

smadbe Sep 12, 2023

GeoffreyHuck Sep 13, 2023 •

edited

Loading

GeoffreyHuck Sep 13, 2023

GeoffreyHuck commented Sep 12, 2023 •

edited

Loading

smadbe Sep 12, 2023

GeoffreyHuck Sep 13, 2023

smadbe Sep 12, 2023

smadbe left a comment

Get permission service: clean "can_request_help" usage #1006

Get permission service: clean "can_request_help" usage #1006

Conversation

GeoffreyHuck commented Sep 8, 2023 • edited Loading

Format of can_request_help_to in response

Performance notice:

codecov bot commented Sep 11, 2023 • edited Loading

Codecov Report

smadbe left a comment

Choose a reason for hiding this comment

Format of can_request_help_to in response

smadbe Sep 11, 2023

Choose a reason for hiding this comment

GeoffreyHuck Sep 12, 2023

Choose a reason for hiding this comment

smadbe Sep 12, 2023

Choose a reason for hiding this comment

GeoffreyHuck Sep 13, 2023 • edited Loading

Choose a reason for hiding this comment

GeoffreyHuck Sep 13, 2023

Choose a reason for hiding this comment

GeoffreyHuck commented Sep 12, 2023 • edited Loading

smadbe Sep 12, 2023

Choose a reason for hiding this comment

GeoffreyHuck Sep 13, 2023

Choose a reason for hiding this comment

smadbe Sep 12, 2023

Choose a reason for hiding this comment

smadbe left a comment

Choose a reason for hiding this comment

GeoffreyHuck commented Sep 8, 2023 •

edited

Loading

Format of `can_request_help_to` in response

codecov bot commented Sep 11, 2023 •

edited

Loading

Format of `can_request_help_to` in response

GeoffreyHuck Sep 13, 2023 •

edited

Loading

GeoffreyHuck commented Sep 12, 2023 •

edited

Loading