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-579: Behat coverage for user with CT limitation adding location #1783

Merged
merged 8 commits into from
Jul 1, 2021

Conversation

micszo
Copy link
Member

@micszo micszo commented Jun 18, 2021

Question Answer
Tickets IBX-579
Bug fix? no
New feature? no (new test case)
BC breaks? no
Tests pass? yes
Doc needed? no
License GPL-2.0

Admin UI part of the test checking whether user with Content Type limitation on content/create policy can add another location.

Related PRs:
ezsystems/BehatBundle#185

Passing build (first job): https://travis-ci.com/github/ezsystems/ezplatform-page-builder/builds/231129567

Checklist:

  • Coding standards ($ composer fix-cs)
  • Ready for Code Review

@micszo micszo changed the title [WIP] IBX-579: Behat coverage for user with CT limitation adding location IBX-579: Behat coverage for user with CT limitation adding location Jun 28, 2021
@micszo micszo marked this pull request as ready for review June 28, 2021 11:21
Copy link
Member

@mnocon mnocon left a comment

Choose a reason for hiding this comment

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

We can't hardcode the root Content Item name, it will fail on different editions.

Other remarks are rather nitpicks, but imho there's no need to divide the location adding action into two separate steps.

@@ -0,0 +1,12 @@
@javascript @addLocation
Feature: Verify that an Editor with Content Type limitation on conten/create policy can add location
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Feature: Verify that an Editor with Content Type limitation on conten/create policy can add location
Feature: Verify that an Editor with Content Type limitation on content/create policy can add location

public function addContentFromUDW($location): void
{
$udw = ElementFactory::createElement($this->context, UniversalDiscoveryWidget::ELEMENT_NAME);
$udw->selectContent('Ibexa Digital Experience Platform/' . $location);
Copy link
Member

Choose a reason for hiding this comment

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

This will fail on Ibexa OSS, where the root Content Item is named Ibexa Platform. (https://github.com/ezsystems/ezplatform-admin-ui/blob/master/src/lib/Behat/Environment/PlatformEnvironmentConstants.php#L17)

And I go to "Content structure" in "Content" tab
And I navigate to content "NewArticle" of type "Article" in root
When I go to "Locations" tab in Content structure of item "NewArticle"
And I click on Add Location button of item "NewArticle"
Copy link
Member

Choose a reason for hiding this comment

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

I'd combine these two steps into one:
I add a new Location to item :itemName under :newLocationPath

If the path is under root (as it's here) use:
I add a new Location to item "NewArticle" under "root/Destination"

@@ -132,6 +133,29 @@ public function navigateToPath(string $path): void
}
}

/**
* @param string $tabName
Copy link
Member

Choose a reason for hiding this comment

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

Please remove redundant doc block.

}

/**
* @param $location
Copy link
Member

Choose a reason for hiding this comment

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

Please remove redundant doc block.

$this->context->getElementByText($tabName, '#ez-tab-list-location-view .ez-tabs__tab')->click();
}

public function addLocation(): void
Copy link
Member

Choose a reason for hiding this comment

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

When the two steps are combined this method cood look like this:

public function addLocation(string $newLocationPath): void
{
    $this->context->findElement('#ez-tab-location-view-locations .ez-table-header__tools .btn--udw-add')->click();
    $udw = ElementFactory::createElement($this->context, UniversalDiscoveryWidget::ELEMENT_NAME);
    $udw->verifyVisibility();
    $udw->selectContent($newLocationPath);
    $udw->confirm();
}

(I've also added a verifyVisibility call to make sure the UDW is loaded correctly before we interact with it.

public function iClickAddLocationButton($item): void
{
$contentItemPage = PageObjectFactory::createPage($this->browserContext, ContentItemPage::PAGE_NAME, $item);
$contentItemPage->addLocation();
Copy link
Member

Choose a reason for hiding this comment

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

Here we need to replace the root keyword with the actual root Content Item name.

Example: https://github.com/ezsystems/ezplatform-admin-ui/blob/3c0cf0eada1136e901036cff91ce8644c3404450/src/lib/Behat/BusinessContext/ContentViewContext.php

It could look like this:

$newLocationPath = $this->argumentParser->replaceRootKeyword($newLocationPath);
$contentItemPage = PageObjectFactory::createPage($this->browserContext, ContentItemPage::PAGE_NAME, $item);
$contentItemPage->addLocation($newLocationPath);

Argument Parser code for reference:
https://github.com/ezsystems/BehatBundle/blob/master/src/lib/Core/Behat/ArgumentParser.php#L60-L72

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the review! All should be applied in 66d0502.

@micszo micszo requested a review from mnocon June 30, 2021 07:20
Copy link
Member

@mnocon mnocon left a comment

Choose a reason for hiding this comment

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

Looks good!

@micszo
Copy link
Member Author

micszo commented Jul 1, 2021

PR for 4.0: #1789

@mnocon mnocon merged commit 1615a28 into ezsystems:2.3 Jul 1, 2021
@micszo micszo deleted the ibx-579-add-location branch July 1, 2021 13:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants