Skip to content
This repository has been archived by the owner on May 3, 2022. It is now read-only.

EZP-27286: Fix BDD to work with content browse #849

Merged
merged 1 commit into from
Jun 12, 2017
Merged

EZP-27286: Fix BDD to work with content browse #849

merged 1 commit into from
Jun 12, 2017

Conversation

StephaneDiot
Copy link
Contributor

@StephaneDiot StephaneDiot commented Apr 18, 2017 •
edited
Loading

JIRA : https://jira.ez.no/browse/EZP-27286

Description

With content tree being removed BDD tests are not up-to-date. I'm adding some tests to work with the new content browse.

@StephaneDiot StephaneDiot force-pushed the Fix_BDD_to_work_with_content_browse branch 22 times, most recently from 003541f to 6bf4806 Compare April 24, 2017 12:48
@StephaneDiot StephaneDiot changed the title WIP Fix BDD to work with content browse WIP EZP-27286: Fix BDD to work with content browse Apr 24, 2017
@StephaneDiot StephaneDiot force-pushed the Fix_BDD_to_work_with_content_browse branch 4 times, most recently from 8c5fc3b to 0b9d48a Compare April 24, 2017 14:53
@StephaneDiot StephaneDiot changed the title WIP EZP-27286: Fix BDD to work with content browse EZP-27286: Fix BDD to work with content browse Apr 25, 2017
@StephaneDiot StephaneDiot force-pushed the Fix_BDD_to_work_with_content_browse branch from 0b9d48a to d5ee895 Compare April 25, 2017 07:50
dpobel
dpobel approved these changes Apr 25, 2017
View reviewed changes
Copy link
Contributor

@dpobel dpobel left a comment

Choose a reason for hiding this comment

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

👍 good work :)

Features/ContentActions/MoveContent.feature Outdated
@@ -18,12 +18,12 @@ Feature: Move content
And I select the "eZ Platform/Older News" folder in the Universal Discovery Widget
And I confirm the selection
Then I am notified that "News Flash" has been moved under "Older News"
And I see "Older News/News Flash" in the content tree
And I do not see "Origin/News flash" in the content tree
And There is a content "Older News/News Flash"
Copy link
Contributor

Choose a reason for hiding this comment

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

I would write

And "Older News/News Flash" content item exists
And "Origin/News flash" content item does not exist

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 even add alternatives to those sentences, even if the implementation is identical:

  • the content item "Older News/Tomorrow news/News Flash" was (not) removed
  • the content item "Older News/Tomorrow news/News Flash" was (not) sent to trash
  • the content item was moved to "Older News/Tomorrow news/News Flash"

They can all use the same theContentItemExists() implementation, but will sound better in the scenarii.

bdunogier
bdunogier suggested changes Apr 26, 2017
View reviewed changes
Features/ContentActions/CopyContent.feature Outdated
@@ -18,7 +18,7 @@ Feature: Copy content
And I select the "eZ Platform/Destination" folder in the Universal Discovery Widget
And I confirm the selection
Then I am notified that "News Flash" has been copied under "Destination"
And I see "Destination/News Flash" in the content tree
And There is a content "Destination/News Flash"
Copy link
Member

Choose a reason for hiding this comment

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

lowercase "T" on there :)

Features/ContentActions/MoveContent.feature Outdated
@@ -18,12 +18,12 @@ Feature: Move content
And I select the "eZ Platform/Older News" folder in the Universal Discovery Widget
And I confirm the selection
Then I am notified that "News Flash" has been moved under "Older News"
And I see "Older News/News Flash" in the content tree
And I do not see "Origin/News flash" in the content tree
And There is a content "Older News/News Flash"
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 even add alternatives to those sentences, even if the implementation is identical:

  • the content item "Older News/Tomorrow news/News Flash" was (not) removed
  • the content item "Older News/Tomorrow news/News Flash" was (not) sent to trash
  • the content item was moved to "Older News/Tomorrow news/News Flash"

They can all use the same theContentItemExists() implementation, but will sound better in the scenarii.

Features/Context/SubContext/CommonActions.php Outdated
*/
public function clickOnTreePath($path)
public function verifyContentIsMissing($path)
Copy link
Member

Choose a reason for hiding this comment

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

The method's name should be adapted to be closer to what is checked (function thereIsNoContent($path)) (including the sentence change suggested by Damien).

Features/Context/SubContext/CommonActions.php Outdated
* @Then There is a content :path
* Explores the finder of the UDW, find the desired element and close the UDW.
*
* @param string $path The content browse path such as 'Content1/Content2/ContentIWantToClick'
Copy link
Member

Choose a reason for hiding this comment

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

No need to add the extra spaces, we don't align statements. And there is nothing to align with :)

Features/Context/SubContext/CommonActions.php Outdated
*
* @param string $path The content browse path such as 'Content1/Content2/ContentIWantToClick'
*/
public function verifyContent($path)
Copy link
Member

Choose a reason for hiding this comment

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

verifyContent isn't a good method name. public function thereIsAContent() is better.

Features/Context/SubContext/CommonActions.php Outdated
/**
* Explores the UDW, expanding it and click on the desired element.
*
* @param string $path The content browse path such as 'Content1/Content2/ContentIWantToClick'
Copy link
Member

Choose a reason for hiding this comment

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

Extra spaces.

Features/Context/SubContext/CommonActions.php Outdated
* @When I cancel the selection
* Cancel the selection in Universal discovery.
*/
public function cancelSelection()
Copy link
Member

Choose a reason for hiding this comment

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

A bit bothered by the fact that UDW sentences and methods are part of CommonActions. Could you try to change that ?

yannickroger
yannickroger previously requested changes Apr 26, 2017
View reviewed changes
Copy link
Contributor

@yannickroger yannickroger left a comment

Choose a reason for hiding this comment

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

Good job. Just have it in 1.9.

.travis.yml
@@ -14,7 +14,7 @@ cache:

matrix:
fast_finish: true
allow_failures:
include:
Copy link
Contributor

Choose a reason for hiding this comment

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

This PR should be opened against 1.9 as you allowed failures there in the first place.

@StephaneDiot StephaneDiot force-pushed the Fix_BDD_to_work_with_content_browse branch from 3080ee1 to 178b940 Compare May 9, 2017 09:10
@StephaneDiot StephaneDiot changed the base branch from master to 1.9 May 9, 2017 09:11
@StephaneDiot StephaneDiot force-pushed the Fix_BDD_to_work_with_content_browse branch 2 times, most recently from ff7627c to b95a3c2 Compare May 9, 2017 09:16
@StephaneDiot StephaneDiot force-pushed the Fix_BDD_to_work_with_content_browse branch from b95a3c2 to c91bba5 Compare May 9, 2017 09:23
@StephaneDiot
Copy link
Contributor Author

updated

@StephaneDiot StephaneDiot mentioned this pull request May 9, 2017
Merged
4 tasks
@dpobel
Copy link
Contributor

dpobel commented May 9, 2017

still ok for me :)
@bdunogier could you have another look please ?

StephaneDiot referenced this pull request Jun 12, 2017
EZP-26992: Travis allow failures to avoid content tree fails
94fa670
@bdunogier bdunogier dismissed yannickroger’s stale review June 12, 2017 08:26

He's not available to do a new round right now.

@StephaneDiot StephaneDiot merged commit 6b32c25 into 1.9 Jun 12, 2017
@StephaneDiot StephaneDiot deleted the Fix_BDD_to_work_with_content_browse branch June 12, 2017 08:33
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Reviewers

@dpobel dpobel dpobel approved these changes

@bdunogier bdunogier bdunogier approved these changes

@yannickroger yannickroger yannickroger left review comments

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@StephaneDiot @dpobel @bdunogier @yannickroger