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

IBX-7717: UDW location endpoint additional permission info #1197

Merged

Conversation

tischsoic
Copy link
Contributor

@tischsoic tischsoic commented Mar 4, 2024

🎫 Issue IBX-7717

Related PRs:

Description:

Frontend note: there's backported loader mixin from 4.6, used in TB to show loader when request is loading.

Example

Request:
path: api/ibexa/v2/location/tree/{locationId}/extended-info

const request = new Request(path, {
        method: 'GET',
        mode: 'same-origin',
        credentials: 'same-origin',
        headers: {
            Accept: 'application/vnd.ibexa.api.ContentTreeExtendedNodeInfo+json',
            'X-Siteaccess': siteaccess,
            'X-CSRF-Token': token,
        },
    });

JSON Response:

{
    "ContentTreeNodeExtendedInfo": {
        "_media-type": "application\/vnd.ibexa.api.ContentTreeNodeExtendedInfo+json",
        "permissions": [
            {
                "_name": "create",
                "hasAccess": true,
                "restrictedContentTypeIdsList": {
                    "restrictedContentTypeIds": [
                        "1",
                        "16"
                    ]
                },
                "restrictedLanguageCodesList": {
                    "restrictedLanguageCodes": [
                        "eng-GB",
                        "eng-US"
                    ]
                }
            },
            {
                "_name": "edit",
                "hasAccess": true,
                "restrictedLanguageCodesList": {
                    "restrictedLanguageCodes": [
                        "eng-GB"
                    ]
                }
            },
            {
                "_name": "delete",
                "hasAccess": false
            },
            {
                "_name": "hide",
                "hasAccess": false
            }
        ]
    }
}

XML Response:

<?xml version="1.0" encoding="UTF-8"?>
<ContentTreeNodeExtendedInfo media-type="application/vnd.ibexa.api.ContentTreeNodeExtendedInfo+xml">
    <function name="create">
        <hasAccess>true</hasAccess>
        <restrictedContentTypeIdsList>
            <value>1</value>
            <value>16</value>
        </restrictedContentTypeIdsList>
        <restrictedLanguageCodesList>
            <value>eng-GB</value>
            <value>eng-US</value>
        </restrictedLanguageCodesList>
    </function>
    <function name="edit">
        <hasAccess>true</hasAccess>
        <restrictedLanguageCodesList>
            <value>eng-GB</value>
        </restrictedLanguageCodesList>
    </function>
    <function name="delete">
        <hasAccess>false</hasAccess>
    </function>
    <function name="hide">
        <hasAccess>false</hasAccess>
    </function>
</ContentTreeNodeExtendedInfo>

Documentation:

This is an internal endpoint, no doc required.

Checklist:

  • Provided PR description.
  • Tested the solution manually.
  • Provided automated test coverage.
  • Checked that target branch is set correctly (master for features, the oldest supported for bugs).
  • Asked for a review (ping @ibexa/engineering).

@tischsoic tischsoic self-assigned this Mar 4, 2024
@GrabowskiM GrabowskiM self-assigned this Mar 8, 2024
@tischsoic tischsoic marked this pull request as ready for review March 11, 2024 08:03
@tischsoic tischsoic requested a review from a team March 11, 2024 08:03
src/lib/REST/Value/ContentTree/NodeExtendedInfo.php Outdated Show resolved Hide resolved
phpstan-baseline.neon Outdated Show resolved Hide resolved
Comment on lines 15 to 16

class LoadExtendedNodeInfoRequest extends BaseParser
Copy link
Member

Choose a reason for hiding this comment

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

  1. final & internal
  2. Naming: ATM the name suggests we're dealing with a value. How about LoadExtendedNodeInfoStructParser (see the next comment for reasons behind "Struct").

Copy link
Contributor Author

@tischsoic tischsoic Mar 18, 2024

Choose a reason for hiding this comment

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

  1. 👍
  2. There is already Ibexa\AdminUi\REST\Input\Parser\ContentTree\LoadSubtreeRequest and I decided to follow with this convention.

Copy link
Contributor

@konradoboza konradoboza Mar 19, 2024

Choose a reason for hiding this comment

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

Point 1 is still not resolved. REST parsers are final across the entire system which is also the case for most of the classes in general.

src/bundle/Controller/Content/ContentTreeController.php Outdated Show resolved Hide resolved
src/bundle/Resources/config/routing_rest.yaml Outdated Show resolved Hide resolved
@@ -34,7 +34,21 @@ public function getContentCreateLimitations(Location $parentLocation): LookupLim
* @throws \Ibexa\Contracts\Core\Repository\Exceptions\InvalidArgumentException
* @throws \Ibexa\Contracts\Core\Repository\Exceptions\NotFoundException
*/
public function getContentUpdateLimitations(Location $parentLocation): LookupLimitationResult;
public function getContentUpdateLimitations(Location $location): LookupLimitationResult;
Copy link
Member

Choose a reason for hiding this comment

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

What is the reason for changing existing contract parameter name?

Copy link
Contributor Author

@tischsoic tischsoic Mar 18, 2024

Choose a reason for hiding this comment

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

Because it didn't match the parameter name from one of the interface's implementations and it did not make much sense to me. As you can see in the original PR, both names did not match each other back then:

It was also later set to parentLocation in 4.6 here: https://github.com/ibexa/admin-ui/pull/872/files#diff-d1039d1a73d7e40070b874e926dbb079bee41c510c4b14ef555ba1d8f7a66f20R32

But I think this should have been called location as done in the initial method implementation: https://github.com/ezsystems/ezplatform-admin-ui/pull/1273/files#diff-c3310f3f33562f15c00a3cfdee25ffbd7f0e88db43c1dea1fe87001dd0ecb785R211.

Copy link
Member

Choose a reason for hiding this comment

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

Please be aware that if someone used named parameters, it would be a BC break. However AFAIK we haven't specified our position on BC for that, so for now I'll accept 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know, I think last time we tentatively agreed that we are not going to have BC on parameters naming.

src/lib/Permission/PermissionChecker.php Outdated Show resolved Hide resolved
src/lib/REST/Value/ContentTree/NodeExtendedInfo.php Outdated Show resolved Hide resolved
@alongosz alongosz requested a review from a team March 15, 2024 15:58
@tischsoic tischsoic force-pushed the IBX-7717-UDW-location-endpoint-additional-permission-info branch 2 times, most recently from f206f08 to 7be0178 Compare March 18, 2024 14:15
@tischsoic tischsoic requested a review from alongosz March 18, 2024 15:09
@alongosz alongosz requested a review from a team March 18, 2024 15:51
Comment on lines 15 to 16

class LoadExtendedNodeInfoRequest extends BaseParser
Copy link
Contributor

@konradoboza konradoboza Mar 19, 2024

Choose a reason for hiding this comment

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

Point 1 is still not resolved. REST parsers are final across the entire system which is also the case for most of the classes in general.

@konradoboza konradoboza requested a review from a team March 19, 2024 08:56
Copy link
Contributor

@tomaszszopinski tomaszszopinski left a comment

Choose a reason for hiding this comment

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

QA approved on IbexaDXP 4.5 commerce.

@alongosz alongosz force-pushed the IBX-7717-UDW-location-endpoint-additional-permission-info branch 2 times, most recently from c8a9853 to 3952e72 Compare April 9, 2024 12:20
@alongosz alongosz requested a review from a team April 9, 2024 12:36
@tischsoic tischsoic force-pushed the IBX-7717-UDW-location-endpoint-additional-permission-info branch from 3952e72 to 4d2b266 Compare April 10, 2024 10:35
Copy link

sonarcloud bot commented Apr 10, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@alongosz alongosz merged commit 3f23e91 into 4.5 Apr 10, 2024
17 checks passed
@alongosz alongosz deleted the IBX-7717-UDW-location-endpoint-additional-permission-info branch April 10, 2024 10:58
@tischsoic
Copy link
Contributor Author

Merged up:

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.

None yet