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-1854: Updated Change Password scenario for 4.0 #357

Merged
merged 1 commit into from
Mar 17, 2022
Merged

IBX-1854: Updated Change Password scenario for 4.0 #357

merged 1 commit into from
Mar 17, 2022

Conversation

micszo
Copy link
Contributor

@micszo micszo commented Mar 9, 2022

Question Answer
Tickets IBX-1854
Bug fix? no
New feature? no
BC breaks? no
Tests pass? yes
Doc needed? no
License GPL-2.0

Change password scenario was originally introduced in https://issues.ibexa.co/browse/IBX-405. Here https://issues.ibexa.co/browse/IBX-1776 the entry was removed from the menu. It was quick-fixed for v4 in 4f9810e.

This PR changes the scenario to use the User Settings page.

Next step is to cover the rest of the User Settings page. I would prefer to treat it with a separate ticket.

Checklist:

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

@micszo micszo marked this pull request as ready for review March 16, 2022 07:32
@micszo micszo changed the title [WIP] IBX-1854: Updated Change Password scenario for 4.0 IBX-1854: Updated Change Password scenario for 4.0 Mar 16, 2022
}

/**
* @Given I switch to :tabName tab
Copy link
Contributor

Choose a reason for hiding this comment

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

Here imo step should be more precise, maybe something like: I switch to :tabName tab in User Settings

Copy link
Contributor

Choose a reason for hiding this comment

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

User Settings is better than the User Preferences I came up with 😄 👍

$this->getHTMLPage()->find($this->getLocator('title'))->assert()->textEquals('User Settings');
}

public function switchTab(string $tabName): void
Copy link
Contributor

Choose a reason for hiding this comment

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

As in UserPreferencesContext, maybe function here can be more precise.

Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO it's fine as it is currently: the usage is $this->userSettingsPage->switchTab(...), which provides enough context about the action that's happening

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 good! Just two small things to consider.

}

/**
* @Given I switch to :tabName tab
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 switch to :tabName tab
* @Given I switch to :tabName tab in User Preferences

I'd add this so that we don't have to worry about clashing step definitions in the future (there can be only one Step with this definition).

We already have similar Steps:

I switch to :tab field group
I switch to :tab tab in Content structure


public function switchTab(string $tabName): void
{
$this->getHTMLPage()->find($this->getLocator($tabName))->click();
Copy link
Contributor

Choose a reason for hiding this comment

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

We can use https://github.com/ibexa/admin-ui/blob/main/src/lib/Behat/Component/TableNavigationTab.php here, example:

public function goToTab(string $tabName)
{
$this->tableNavigationTab->goToTab($tabName);
}

@micszo micszo merged commit 22ec4c7 into main Mar 17, 2022
@micszo micszo deleted the ibx-1854 branch March 18, 2022 07:23
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.

None yet

4 participants