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-6330: [Behat] Cover "My Drafts" Page #878

Merged
merged 9 commits into from
Aug 25, 2023
Merged

IBX-6330: [Behat] Cover "My Drafts" Page #878

merged 9 commits into from
Aug 25, 2023

Conversation

jwibexa
Copy link
Contributor

@jwibexa jwibexa commented Aug 22, 2023

Question Answer
Tickets https://issues.ibexa.co/browse/IBX-6330
Bug fix? no
New feature? yes
BC breaks? yes/no
Tests pass? yes/no
Doc needed? yes/no
License GPL-2.0

Checklist:

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

Copy link
Contributor

@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 very, very promising, great start! 🎖️


Scenario: It is possible to delete a draft
Given I create "article" Content drafts
| title | short_title | parentPath | language |
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: it would be best if the columns were aligned for better readability

@@ -56,7 +56,7 @@ services:

Ibexa\AdminUi\Behat\Page\ChangePasswordPage: ~

Ibexa\AdminUi\Behat\Page\UserSettingsPage: ~
Ibexa\AdminUi\Behat\Page\MyDraftsPage: ~
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice catch finding that the UserSettingPage service definition is doubled 💪

*/
public function iSeeTheDraftIsDeleted(string $draftName): void
{
assert($this->myDraftsPage->doSeeDraft($draftName));
Copy link
Contributor

Choose a reason for hiding this comment

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

In our case we don't want to see the draft - we expect doSeeDraft to return false. Also we use PHPUnit's assertions, so the code could look like this:

Assert::assertFalse($this->myDraftsPage->doSeeDraft($draftName))

*/
public function iClickEdit(string $draftName): void
{
$this->myDraftsPage->clickEditDraft($draftName);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
$this->myDraftsPage->clickEditDraft($draftName);
$this->myDraftsPage->edit($draftName);

It's better to avoid naming the action (clicking, hovering, ...) directly - it's the intention that matters the most (editing), not the action (clicking). The action can change in future redesigns, avoiding naming it makes our lives a little easier

}

/**
* @Given I click edit :draftName
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* @Given I click edit :draftName
* @Given I edit :draftName draft on MyDrafts page

I'm afraid that a I click edit :string step definition is too generic

| Short title | TestMyDraftSavePublish |
| Intro | TestMyDraftIntro |
And I click on the edit action bar button "Save"
Then I should be on Content update page for "TestMyDraftSavePublish"
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: missing newline

}

public function verifyIsLoaded(): void
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Please fill in this method - we can make sure that the correct page title (header) is displayed.

$this->dialog->confirm();
}

public function doSeeDraft(string $draftName): bool
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public function doSeeDraft(string $draftName): bool
public function isDraftOnTheList(string $draftName): bool

We can use https://github.com/ibexa/admin-ui/blob/main/src/lib/Behat/Component/Table/TableInterface.php#L15 here, meaning that we should be able to do this:

public function isDraftOnTheList(string $draftName): bool
{
    return $this->table->hasElement(['Name' => $draftName]);
}

Copy link
Contributor

@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 great, thanks! 😎

| TestMyDraft | TestMyDraft | root | eng-GB |
And I am logged as admin
And I open "MyDrafts" page in admin SiteAccess
When I click edit "TestMyDraft" on MyDrafts page
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
When I click edit "TestMyDraft" on MyDrafts page
When I edit "TestMyDraft" on MyDrafts page

Just one small suggestion

@sonarcloud
Copy link

sonarcloud bot commented Aug 24, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@katarzynazawada
Copy link
Contributor

What about 4.5 version? Shouldn't we add those tests there as well? 🙂

@mnocon
Copy link
Contributor

mnocon commented Aug 24, 2023

When creating this task I thought that 4.6 is enough, it makes the task a bit easier (no need for upmerge changes etc. - and this is the first task for Kuba).

We can backport it as a follow-up, what do you think?

Copy link
Contributor

@micszo micszo left a comment

Choose a reason for hiding this comment

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

Great start! 🚀

@micszo micszo changed the title IBX - 6330 Cover "My Drafts" Page IBX-6330: [Behat] Cover "My Drafts" Page Aug 25, 2023
@micszo micszo merged commit b0e1549 into main Aug 25, 2023
16 checks passed
@micszo micszo deleted the behat-training branch August 25, 2023 09:01
jwibexa added a commit that referenced this pull request Aug 30, 2023
* Initial implementation

* FInished the task of testing draft deletion

* Added the test for the edition of draft, splited the tests into 2 versions.

* cleanup

* Improvemtn of the edit table selector

* Fixes regarding editing draft selectors

* code style check fix

* Fixes regarding veryfiing deletion, style fixes, veryfiisloaded added

* Added fix regarding not finding delete confirmation, naming fix

---------

Co-authored-by: Marek Nocoń <mnocon@users.noreply.github.com>
(cherry picked from commit b0e1549)
micszo pushed a commit that referenced this pull request Sep 11, 2023
* IBX-6330: [Behat] Cover "My Drafts" Page (#878)

* Initial implementation

* FInished the task of testing draft deletion

* Added the test for the edition of draft, splited the tests into 2 versions.

* cleanup

* Improvemtn of the edit table selector

* Fixes regarding editing draft selectors

* code style check fix

* Fixes regarding veryfiing deletion, style fixes, veryfiisloaded added

* Added fix regarding not finding delete confirmation, naming fix

---------

Co-authored-by: Marek Nocoń <mnocon@users.noreply.github.com>
(cherry picked from commit b0e1549)

* Cover "My Drafts" Page

* Cleanup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants