FIX Resolve deduping problem with group codes. #10333
Conversation
GuySartorelli
force-pushed
the
pulls/4.10/fix-group-code-dedupe
branch
from
306bdfa
to
8e9956b
Compare
| @@ -522,16 +513,6 @@ public function validate() | |||
| ->map('Code', 'Title') | |||
| ->toArray(); | |||
|
|
|||
| if (isset($currentGroups[$this->Code])) { | |||
There was a problem hiding this comment.
I'm just wondering if it's the correct approach to delete validation and just increment groups?
Is the reason why we are deleting the validation is because it's just never worked? Or it used to work and we accidentally broke it with a recent PR?
If it never worked then it makes sense to delete this code block (or at least move it to a private method for now link it back up in a future major). Otherwise wouldn't the correct fix here be to fix the broken validation rather than incrementing duplicate groups?
(I can see how enforcing validation when programatically setting a group would be difficult)
There was a problem hiding this comment.
delete validation and just increment groups
That's what's already happening. This validation code doesn't get reached at all because the code is already incremented in setCode() (prior to this PR).
Is the reason why we are deleting the validation is because it's just never worked? Or it used to work and we accidentally broke it with a recent PR?
Yes, we're deleting it because it never worked. It was added at the same time as the code in setCode() which removed deduping. Now that I'm moving the deduping logic to onBeforeWrite() the validation would kick in if I kept it here, and the deduping wouldn't happen, which would be a breaking change.
Based on the comments in the issue and in the original PR, it sounds like the idea was always to add deduping - there's no clear reason why this validation code was added in the first place but it just wasn't being called in any case.
GuySartorelli
force-pushed
the
pulls/4.10/fix-group-code-dedupe
branch
from
8e9956b
to
8fe54da
Compare
|
The validation of group title that was put in place with that PR is still there. Both before and after the changes I'm proposing the following will be true:
|
Also remove dead validation code.
GuySartorelli
force-pushed
the
pulls/4.10/fix-group-code-dedupe
branch
from
8fe54da
to
e0c4f01
Compare
There was a problem hiding this comment.
OK, makes sense.
Looking back at the issue that proceeded this bug issue, I'm thinking we may need to broaden the scope here as we have a different set of rules for UI vs programatically. Perhaps we need to:
a) Add the same incrementing logic to Title if title is not blank
b) Add behat test to ensure that blank title throws validation warning in UI
c) Add unit test to ensure that blank Title is allowed when group is created programmatically
It may make sense to split this off handling duplicate Title as a separate issue? What do you think?
|
I think the title stuff is out of scope for this bugfix (This PR is about the Note that as far as deduping the title is concerned: There was validation added in that same PR in |
There was a problem hiding this comment.
OK seems good
The unit test covers the programatically created group.
I did a front-end test to check the UI Title validation works, I then created a group and immediatly changed the Title, then created a new group with the original Title (so that it would have a duplicate Code as the first group), in the database it suffixed the new group code with -2
|
@GuySartorelli You should release a new patch version for 4.10.* and merge up to 4.11 (don't release as it's in beta, we can release 4.11.1 shortly after stable), and also merge-up to 4 |
|
Tagged and merged up |
Group code deduping was introduced with #10113 but it had the unexpected side effect of changing a group code if the same code was set a second time on an existing group. Reproduction steps are in the parent issue.
As discussed in the parent issue I have moved the deduping logic to
onBeforeWrite()- this makes it more robust as we know at write time that the code is unique. We could otherwise get into a situation where we are creating two new groups at the same time and they both end up with the same code.There was also some validation added in that PR to throw a validation error if the code was duplicated - but because of the code deduping the validation error could never be thrown. I have removed that validation logic.
Parent issue