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

Manage event response in show and index action to be able to redirect #287

Merged
merged 3 commits into from
Jan 7, 2022
Merged

Manage event response in show and index action to be able to redirect #287

merged 3 commits into from
Jan 7, 2022
+81 −2

Conversation

maximehuran
Copy link
Contributor

@maximehuran maximehuran commented Feb 23, 2021 •
edited
Loading

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? o
Related tickets mentioned in Sylius/Sylius#10286
License MIT

@maximehuran maximehuran requested a review from a team as a code owner February 23, 2021 15:43
@maximehuran maximehuran force-pushed the feature/manage-show-index-events branch 2 times, most recently from 3dad002 to d5cb982 Compare February 23, 2021 15:47
lchrusciel
lchrusciel reviewed Apr 19, 2021
View reviewed changes
Copy link
Contributor

@lchrusciel lchrusciel left a comment

Choose a reason for hiding this comment

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

Hey Maxime 👋

thanks for the contribution. Can you also provide some tests for this behavior? The best combo would be to have Spec + functional test in PHPUnit

@maximehuran maximehuran force-pushed the feature/manage-show-index-events branch 3 times, most recently from 88c1fd0 to 68af98b Compare May 24, 2021 08:30
@maximehuran maximehuran requested a review from lchrusciel May 24, 2021 08:30
@maximehuran
Copy link
Contributor Author

Hi, I added some PHP Spec tests. I am not sure about it but I don't know how to setup the test application for this plugin.
I don't see PHP Unit tests actually managing some redirection on this plugin.

@loic425
Copy link
Member

loic425 commented May 24, 2021 •
edited
Loading

Hi, I added some PHP Spec tests. I am not sure about it but I don't know how to setup the test application for this plugin.
I don't see PHP Unit tests actually managing some redirection on this plugin.

Just look at the CI to see how it is set up :)

@maximehuran
Copy link
Contributor Author

I would like to run the CI to check my tests, I think it's the easiest solution

@maximehuran
Copy link
Contributor Author

ping @lchrusciel I added PHP Spec tests :)

@Zales0123 Zales0123 added the Feature New feature proposals. label Jan 7, 2022
@Zales0123 Zales0123 force-pushed the feature/manage-show-index-events branch from 68af98b to 812e698 Compare January 7, 2022 08:36
Zales0123
Zales0123 approved these changes Jan 7, 2022
View reviewed changes
Copy link
Contributor

@Zales0123 Zales0123 left a comment

Choose a reason for hiding this comment

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

Hello Maxime 🖖 I've rebased your PR, let's see how the build passes, but it's good to go in my opinion :)

Zales0123
Zales0123 reviewed Jan 7, 2022
View reviewed changes
src/Bundle/spec/Controller/ResourceControllerSpec.php Outdated
$event->getResponse()->willReturn($response);

$configuration->isHtmlRequest()->shouldNotBeCalled();
$this->createRestView($configuration, $resource)->shouldNotBeCalled();
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, ok, this is the problem 💃 createRestView is a protected function inside ResourceController, so we should rather check if $this->viewHandler is called or not 🖖

Copy link
Contributor

Choose a reason for hiding this comment

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

I've allowed myself to fix it :) 🚀

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you Mateusz you're the king !

@Zales0123 Zales0123 merged commit 72a3877 into Sylius:master Jan 7, 2022
@Zales0123
Copy link
Contributor

Thanks, Maxime! 🎉

@maximehuran maximehuran deleted the feature/manage-show-index-events branch July 6, 2023 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Zales0123 Zales0123

@jacquesbh jacquesbh

@lchrusciel lchrusciel

Assignees
No one assigned
Labels
Feature New feature proposals.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@maximehuran @loic425 @Zales0123 @jacquesbh @lchrusciel