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

[ReviewBundle] Recalculate subject rating on review post create event #8678

Merged
merged 7 commits into from Oct 4, 2017

Conversation

Projects
None yet
6 participants
@loic425
Contributor

loic425 commented Sep 15, 2017

Q A
Bug fix? kind of
New feature? kind of
BC breaks? no
Related tickets
License MIT

In case you handle your reviews without moderation (directly with accepted status), you need your subject would be recalculate on post create event. You also need this feature if you handle ratings without title and comment.

@Zales0123

This comment has been minimized.

Show comment
Hide comment
@Zales0123

Zales0123 Sep 18, 2017

Member

Shouldn't this be implemented inside a custom application? I think this is a custom feature and should not be provided together with default configuration, it's a different behavior :)
I think that instead of implementing this, you can write a cookbook how to customize product reviews recalculation? :) /cc @GSadee @CoderMaggie

Member

Zales0123 commented Sep 18, 2017

Shouldn't this be implemented inside a custom application? I think this is a custom feature and should not be provided together with default configuration, it's a different behavior :)
I think that instead of implementing this, you can write a cookbook how to customize product reviews recalculation? :) /cc @GSadee @CoderMaggie

@loic425

This comment has been minimized.

Show comment
Hide comment
@loic425

loic425 Sep 18, 2017

Contributor

@Zales0123 In sylius, this recalculates for nothing but I think it adds useless complexity on this bundle for a common use case. Indeed, I can write a cookbook to remove moderation on a review if you prefer.

Contributor

loic425 commented Sep 18, 2017

@Zales0123 In sylius, this recalculates for nothing but I think it adds useless complexity on this bundle for a common use case. Indeed, I can write a cookbook to remove moderation on a review if you prefer.

@pamil

This comment has been minimized.

Show comment
Hide comment
@pamil

pamil Sep 18, 2017

Member

I'd say the current behaviour is buggy, eg. the current average rating won't be even be calculated when using fixtures or when programatically adding already accepted reviews to the product.

Member

pamil commented Sep 18, 2017

I'd say the current behaviour is buggy, eg. the current average rating won't be even be calculated when using fixtures or when programatically adding already accepted reviews to the product.

@pamil

This comment has been minimized.

Show comment
Hide comment
@pamil

pamil Sep 18, 2017

Member

Anyway, the other thing is that we rely on ResourceBundle's events, so the cases mentioned above wouldn't be handled with this PR, but then making review already accepted would be a no-brainer, currently you might spend quite some time figuring out why average rating isn't updated.

Member

pamil commented Sep 18, 2017

Anyway, the other thing is that we rely on ResourceBundle's events, so the cases mentioned above wouldn't be handled with this PR, but then making review already accepted would be a no-brainer, currently you might spend quite some time figuring out why average rating isn't updated.

@loic425

This comment has been minimized.

Show comment
Hide comment
@loic425

loic425 Sep 18, 2017

Contributor

@pamil
We can use doctrine lifecycle events instead, so the fixtures would be ok. What do you think ?

Contributor

loic425 commented Sep 18, 2017

@pamil
We can use doctrine lifecycle events instead, so the fixtures would be ok. What do you think ?

@pamil pamil added this to the 1.0 milestone Sep 20, 2017

@loic425

This comment has been minimized.

Show comment
Hide comment
@loic425

loic425 Sep 27, 2017

Contributor

@pamil "but then making review already accepted would be a no-brainer"
If you need to implement simple ratings without comments, it make sense.
Do you want me to make another pull-request with lifecycle events instead of resource events and you can choose which one could be merged ? I think lifecycle events would be better to test average rating with behat.

Contributor

loic425 commented Sep 27, 2017

@pamil "but then making review already accepted would be a no-brainer"
If you need to implement simple ratings without comments, it make sense.
Do you want me to make another pull-request with lifecycle events instead of resource events and you can choose which one could be merged ? I think lifecycle events would be better to test average rating with behat.

@pamil

This comment has been minimized.

Show comment
Hide comment
@pamil

pamil Sep 27, 2017

Member

@loic425 sorry for the delay, must've marked this PR as read by an accident. The current version is all right for me (although while being a bugfix, it should target 1.0 branch).

If you're willing to make it use Doctrine lifecycle events, that's even better - it would be easier to test as well! :)

Member

pamil commented Sep 27, 2017

@loic425 sorry for the delay, must've marked this PR as read by an accident. The current version is all right for me (although while being a bugfix, it should target 1.0 branch).

If you're willing to make it use Doctrine lifecycle events, that's even better - it would be easier to test as well! :)

@loic425

This comment has been minimized.

Show comment
Hide comment
@loic425

loic425 Sep 28, 2017

Contributor

@pamil I'm on it, build is failed but i'm working on fixing phpspec and behat scenarios.

Contributor

loic425 commented Sep 28, 2017

@pamil I'm on it, build is failed but i'm working on fixing phpspec and behat scenarios.

@loic425

This comment has been minimized.

Show comment
Hide comment
@loic425

loic425 Sep 29, 2017

Contributor

@pamil I think it's ok:) All changes have been done.

Contributor

loic425 commented Sep 29, 2017

@pamil I think it's ok:) All changes have been done.

@pamil

Just that one issue and it's good to go! 🎉

@loic425

This comment has been minimized.

Show comment
Hide comment
@loic425

loic425 Sep 29, 2017

Contributor

@pamil Done! 🥇

Contributor

loic425 commented Sep 29, 2017

@pamil Done! 🥇

@pamil

pamil approved these changes Sep 29, 2017

@GSadee

GSadee approved these changes Oct 2, 2017

@pjedrzejewski pjedrzejewski changed the base branch from master to 1.0 Oct 3, 2017

@pjedrzejewski

This comment has been minimized.

Show comment
Hide comment
@pjedrzejewski

pjedrzejewski Oct 3, 2017

Member

@loic425 Thanks for your work, could you rebase against 1.0 branch? This is a bug fix and should be merged to 1.0 but after changing the base we have some conflicts. :)

Member

pjedrzejewski commented Oct 3, 2017

@loic425 Thanks for your work, could you rebase against 1.0 branch? This is a bug fix and should be merged to 1.0 but after changing the base we have some conflicts. :)

@loic425

This comment has been minimized.

Show comment
Hide comment
@loic425

loic425 Oct 3, 2017

Contributor

@pjedrzejewski done :)

Contributor

loic425 commented Oct 3, 2017

@pjedrzejewski done :)

@pjedrzejewski pjedrzejewski merged commit 0541f42 into Sylius:1.0 Oct 4, 2017

2 checks passed

Scrutinizer 3 updated code elements
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@pjedrzejewski

This comment has been minimized.

Show comment
Hide comment
@pjedrzejewski

pjedrzejewski Oct 4, 2017

Member

Thanks Loïc! 👍

Member

pjedrzejewski commented Oct 4, 2017

Thanks Loïc! 👍

@loic425 loic425 deleted the loic425:improve/recalculate-subject-rating-on-review-post-create branch Feb 26, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment