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.

Sign up for GitHub

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

Jump to bottom

DEP PHP Support in CMS5 #693

Merged
merged 1 commit into from
Jan 12, 2023
Merged

DEP PHP Support in CMS5 #693

merged 1 commit into from
Jan 12, 2023

Conversation

sabina-talipova
Copy link
Contributor

@sabina-talipova sabina-talipova mentioned this pull request Dec 9, 2022
Closed
3 tasks
@sabina-talipova
Copy link
Contributor Author

sabina-talipova commented Dec 21, 2022 •
edited
Loading

Requires silverstripe/silverstripe-widgets#193 and silverstripe/silverstripe-comments#354

@sabina-talipova sabina-talipova force-pushed the pulls/4/upgrade-cms5 branch 3 times, most recently from c9d114d to ed3321c Compare January 9, 2023 23:19
@sabina-talipova
Copy link
Contributor Author

Created new card for failed Behat tests #696

maxime-rainville
maxime-rainville reviewed Jan 12, 2023
View reviewed changes
Copy link

@maxime-rainville maxime-rainville left a comment

Choose a reason for hiding this comment

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

Looks fine. Just curious about why the changes to the test were needed.

tests/php/BlogFunctionalTest.php Show resolved Hide resolved
tests/php/BlogFunctionalTest.php Outdated
@@ -23,22 +21,25 @@ protected function setUp(): void

public function testBlogWithMultibyteUrl()
{
$result = $this->get(rawurlencode('آبید'));
$this->logInWithPermission('ADMIN');
Copy link

Choose a reason for hiding this comment

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

Why was this changed?

Copy link

Choose a reason for hiding this comment

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

Suggested change
$this->logInWithPermission('ADMIN');
$this->logInWithPermission('VIEW_DRAFT_CONTENT');

Copy link
Contributor Author

Choose a reason for hiding this comment

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

User doesn't have an access to the Draft page without Admin permission

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DONE

maxime-rainville
maxime-rainville requested changes Jan 12, 2023
View reviewed changes
Copy link

@maxime-rainville maxime-rainville left a comment

Choose a reason for hiding this comment

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

Answered my own question.

I just think we should use the more minimal VIEW_DRAFT_CONTENT permission to run the test.

tests/php/BlogFunctionalTest.php Outdated
@@ -23,22 +21,25 @@ protected function setUp(): void

public function testBlogWithMultibyteUrl()
{
$result = $this->get(rawurlencode('آبید'));
$this->logInWithPermission('ADMIN');
Copy link

Choose a reason for hiding this comment

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

Suggested change
$this->logInWithPermission('ADMIN');
$this->logInWithPermission('VIEW_DRAFT_CONTENT');

tests/php/BlogFunctionalTest.php Outdated

$this->assertEquals(200, $result->getStatusCode());
}

public function testMemberProfileWithMultibyteUrlAndName()
{
$result = $this->get(rawurlencode('آبید') . '/profile/' . rawurlencode('عبّاس-آبان'));
$this->logInWithPermission('ADMIN');
Copy link

Choose a reason for hiding this comment

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

Suggested change
$this->logInWithPermission('ADMIN');
$this->logInWithPermission('VIEW_DRAFT_CONTENT');

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DONE

tests/php/BlogFunctionalTest.php Outdated

$this->assertEquals(200, $result->getStatusCode());
$this->assertStringContainsString('My Blog Post', $result->getBody());
}

public function testMemberProfileWithMultibyteUrlAndEnglishName()
{
$result = $this->get(rawurlencode('آبید') . '/profile/bob-jones');
$this->logInWithPermission('ADMIN');
Copy link

Choose a reason for hiding this comment

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

Suggested change
$this->logInWithPermission('ADMIN');
$this->logInWithPermission('VIEW_DRAFT_CONTENT');

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DONE

tests/php/Model/BlogControllerFunctionalTest.php Outdated
@@ -23,15 +21,17 @@ protected function setUp(): void

public function testGetCategoriesWithMultibyteUrl()
{
$result = $this->get('my-blog/category/' . rawurlencode('آبید'));
$this->logInWithPermission('ADMIN');
Copy link

Choose a reason for hiding this comment

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

Suggested change
$this->logInWithPermission('ADMIN');
$this->logInWithPermission('VIEW_DRAFT_CONTENT');

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DONE

tests/php/Model/BlogControllerFunctionalTest.php Outdated

$this->assertEquals(200, $result->getStatusCode());
$this->assertStringContainsString('آبید', $result->getBody());
}

public function testGetTagsWithMultibyteUrl()
{
$result = $this->get('my-blog/tag/' . rawurlencode('برتراند'));
$this->logInWithPermission('ADMIN');
Copy link

Choose a reason for hiding this comment

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

Suggested change
$this->logInWithPermission('ADMIN');
$this->logInWithPermission('VIEW_DRAFT_CONTENT');

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DONE

maxime-rainville
maxime-rainville approved these changes Jan 12, 2023
View reviewed changes
Copy link

@maxime-rainville maxime-rainville left a comment

Choose a reason for hiding this comment

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

LGTM

@maxime-rainville maxime-rainville merged commit 0f1da8c into silverstripe:4 Jan 12, 2023
@maxime-rainville maxime-rainville deleted the pulls/4/upgrade-cms5 branch January 12, 2023 22:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maxime-rainville maxime-rainville maxime-rainville approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@sabina-talipova @maxime-rainville