-
Notifications
You must be signed in to change notification settings - Fork 379
🐛Link role update #1287
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
base: main
Are you sure you want to change the base?
🐛Link role update #1287
Conversation
a5c82b0
to
708cefa
Compare
7b40277
to
95c4e06
Compare
I find a bit strange to have docs/src/backend/core/models.py Lines 370 to 377 in 95c4e06
|
src/backend/core/api/serializers.py
Outdated
link_role = attrs.get("link_role") | ||
|
||
# If link_reach is restricted, link_role should not be set to anything meaningful | ||
if link_reach == models.LinkReachChoices.RESTRICTED and link_role in [ |
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 think you should use the existing get_select_options method to ensure unicity of the logic. Also, should we add something to abilities for the frontend to know? This makes me wonder how did you bump into this and was it really a functional issue because actually, link_role is ignored and irrelevant when link_reach is restricted.
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, actually I was misleaded, the root problem is how max_role
is computed when a child is still in a restricted type (after being created).
docs/src/backend/core/choices.py
Lines 106 to 113 in 0775752
max_role = max( | |
( | |
link["link_role"] | |
for link in ancestors_links | |
if link["link_reach"] == max_reach | |
), | |
key=LinkRoleChoices.get_priority, | |
) |
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 I understand the problem correctly from the issue, the backend is correct but the frontend should set link_role to authenticated AND link_role to edit for it to be overriden.... doing what tou propose would lead to side effects and the problem would still be the same downstream or for authenticated vs public...
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.
Said differently: you can't expect to override an authenticated computed role by setting a different role for another level of reach!!
Technically in the frontend, you should use the computed link_reach to set your override and not the current document link reach
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.
And I agree that there should be a validation to make sure that only allowed combinations are passed as per get_select_options
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.
Technically in the frontend, you should use the computed link_reach to set your override and not the current document link reach
The front does not set the current document link reach.
Request URL https://docs.numerique.gouv.fr/api/v1.0/documents/[DOC_ID]/link-configuration/
Request Method PUT
Payload:{"link_role": "editor"}
But yes, if we force the link_reach
in the payload with the computed one, the problem does not occur anymore.
I suppose link_reach
should be mandatory so to avoid mismatch, plus yes a validation to not be able to have these kind of payload: {"link_reach": "restricted", "link_role": "editor"}
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 adapted the serializer validation with what was made initially, I think it covers all the cases.
Feel free to take over.
492b16f
to
7f2b28a
Compare
src/backend/core/choices.py
Outdated
for link in ancestors_links | ||
if (link["link_reach"] == max_reach or | ||
link["link_reach"] == LinkReachChoices.RESTRICTED) | ||
] |
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 link rôle of a restricted document is irrelevant so why take it into account? Can you try explaining in PR description what is the functional problem you face?
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 tried to be more descriptive.
By following the issue #1283, you can reproduce the problem in production.
7f2b28a
to
6125d0a
Compare
When a document was restricted, the link role could be updated from "link-configuration" and gives a 200 response, but the change did not have any effect because of a restriction in LinkReachChoices. We added a validation step to ensure that the link role can only be updated if the document is not restricted.
By default a document is "restricted", a restricted document cannot have a role "editor" or "reader". With inheritance, a child document could have a computed link reach different than "restricted" though. We pass the computed link reach when we update the link role, to be sure if follows the parent computed link reach.
6125d0a
to
8780fc1
Compare
8780fc1
to
2c4a906
Compare
Purpose
When a child document is
restricted
, but have a different computation (authenticated
), the link role could be updated fromlink-configuration
and gives a 200 response, but the change did not have any effect on the computation because of thelink_reach
being stillrestricted
.See: #1283
Proposal
We added some validations steps to be sure the
link-configuration
can be updated correctly.We adapted the front to follow the new types requirement on
link-configuration
(link reach mandatory).🥅(backend) link role could be updated when restricted document
🩹(frontend) add computed_link_reach on PUT link-configuration