[Reviews] nullable title for reviews #9212
Conversation
loic425
commented
Feb 26, 2018
| Q | A |
|---|---|
| Branch? | master |
| Bug fix? | no |
| New feature? | yes |
| BC breaks? | no |
| Deprecations? | no |
| Related tickets | fixes #8172 |
| License | MIT |
| <constraint-mapping xmlns="http://symfony.com/schema/dic/constraint-mapping" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://symfony.com/schema/dic/constraint-mapping http://symfony.com/schema/dic/services/constraint-mapping-1.0.xsd"> | ||
| <class name="Sylius\Component\Review\Model\Review"> | ||
| <property name="title"> | ||
| <constraint name="NotBlank"> |
There was a problem hiding this comment.
Shouldn't this constraint be removed now?
There was a problem hiding this comment.
I don't think so. IMHO, when you write a commented review, you need a title and a comment. When you have a "rating system" by only clicking on stars for example, title is not required.
There was a problem hiding this comment.
Does it work like that now (title is required if comment is filled)? I guess we'd need some Behat scenario to cover that.
There was a problem hiding this comment.
No, I just kept existing behaviour on sylius core :)
We can go further but it's more complex. For exemple, you need to separate ratings from commented reviews on frontend. Moreover, the status workflow is different. A single rate doesn't need to be accepted.
There was a problem hiding this comment.
@pamil I think it's just a first step. It's just an improvment for sylius review bundle and an easier way to improve/customize sylius review behaviour in a sylius shop.
There was a problem hiding this comment.
@Zales0123 @pamil @pjedrzejewski @lchrusciel
Could you think about that, discuss this and tell me what do you think about the best behaviour I could implement.
Currently, with existing Pull request, I added a new validation group (sylius_review) on title to allow using only "sylius" validation group to remove not blank validation on title. Another solution could be the idea suggested by Pamil: title is required only if comment is filled. I guess it needs a form listener to do that.
There was a problem hiding this comment.
Sorry @loic425 for the long response. I thing that requiring comment only if title is filled is the best approach, just seems as the most natural solution. /cc @pamil @pjedrzejewski @lchrusciel
There was a problem hiding this comment.
@Zales0123 Ok, I'm gonna work on that approach.
lchrusciel
left a comment
There was a problem hiding this comment.
I'm ok with the current implementation, but IMHO the simplest removal of this constraint will work totally fine.
pamil
left a comment
There was a problem hiding this comment.
I think we can merge this one and providing a conditional validation (title required if there's a comment) would be the next step then.
|
Thanks Loic! 🥇 |
|
I've created #9239 to track that conditional validation feature. |
…itle [Reviews] nullable title for reviews