Skip to content
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

fix: use available CTs instead of only readable #1562

Merged
merged 2 commits into from
Dec 20, 2023

Conversation

SofiaMargariti
Copy link
Contributor

@SofiaMargariti SofiaMargariti commented Dec 15, 2023

The way reference field works, it will take the available CTs for the field - via validations - filter out the ones that user has no permissions to read and pass the remaining list to the Entity Search modal. If an empty array of CTs is provided Entity Search will render all CTs of the environment.

When a user has partial access to content types e.g. via tags these content types will not appear in the list of readable CTs and will result in an empty array to be passed to Entity Search which will then result in all available CTs to be listed overriding the field validations.

If instead we pass the available CTs then Entity Search will still respect the validations and is smart enough to filter out individual entities the user might not have permissions to read.

See the following example:

  • fields allows only Component:Text entries to be linked via validations
  • the user has access to some entries of the Component:Text via tags but cannot read any entry of that CT

Before all CTs would be listed:
Screenshot 2023-12-15 at 16 38 15
After, only the Component:Text is listed and only the entries that user can read:
Screenshot 2023-12-15 at 16 46 40

Related tickets:

@SofiaMargariti SofiaMargariti marked this pull request as ready for review December 18, 2023 08:10
@SofiaMargariti SofiaMargariti requested a review from a team as a code owner December 18, 2023 08:10
Copy link
Contributor

@Chaoste Chaoste left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice fix. If you have time for this, I would appreciate a small comment next to the code, so we don't break this later again

Copy link
Member

@z0al z0al left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry to block but just want to make sure I understand this. it seems readableContentTypes are only ever used in the places you changed in this PR, so does that mean we should remove them from the reference editor props?

Also, any reason why not to fix this on the app level?

@SofiaMargariti
Copy link
Contributor Author

SofiaMargariti commented Dec 19, 2023

Sorry to block but just want to make sure I understand this. it seems readableContentTypes are only ever used in the places you changed in this PR, so does that mean we should remove them from the reference editor props?

Good point @z0al, I didn't notice they are not used anymore, we could remove them in this case.

Also, any reason why not to fix this on the app level?

Do you mean the web app here? This logic is all encapsulated inside the field-editor itself, it essentially determines the CTs to show based on field validations and permissions, they cannot be overridden from the outside atm and introducing new parameters seemed that it would complicate the logic even more. Do you have another suggestion?

@z0al
Copy link
Member

z0al commented Dec 19, 2023

Do you mean the web app here? This logic is all encapsulated inside the field-editor itself

You're right @SofiaMargariti , I was under the impression that the list is passed down from the host app but I was wrong. In that case, let's remove please the readableContentTypes related logic and 🚢

@SofiaMargariti SofiaMargariti merged commit fa5ddce into master Dec 20, 2023
14 checks passed
@SofiaMargariti SofiaMargariti deleted the fix/available-cts branch December 20, 2023 07:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants