allow group named "0" to be deleted #29310
Conversation
individual-it
requested review from
PVince81,
SergioBertolinSG and
phil-davis
phil-davis
left a comment
phil-davis
left a comment
There was a problem hiding this comment.
Suggestion for enhancing the test scenario.
There was a problem hiding this comment.
s/theseGroupsShouldNotExists/theseGroupsShouldNotExist/
There was a problem hiding this comment.
The scenario could also add some groups that are not deleted, and check that they are still listed and still exist at the end. That will help confirm that the delete buttons on the UI do delete the expected group, and not some random group 1 above or below the button, for example.
individual-it
force-pushed
the
fix-grpname-0
branch
from
43f30e7 to
9cf6f17
Compare
Codecov Report
@@ Coverage Diff @@
## master #29310 +/- ##
=========================================
Coverage 59.59% 59.59%
Complexity 17170 17170
=========================================
Files 1029 1029
Lines 57239 57239
=========================================
Hits 34113 34113
Misses 23126 23126Continue to review full report at Codecov.
|
|
fixed the typos and wrote some more tests |
PVince81
left a comment
PVince81
left a comment
There was a problem hiding this comment.
👍 for the JS changes, not sure about the UI tests
| var $li = $userGroupList.find('.isgroup:last-child').clone(); | ||
| $li | ||
| .data('gid', gid) | ||
| .attr('data-gid', gid) |
There was a problem hiding this comment.
not sure if needed. Setting the data isn't the problem, it's reading that would cause auto-parse-datatype when using .data.
There was a problem hiding this comment.
it gives funny visual problems without, as it only clones the last li and the attr stays the same as in the last li.
Without this change, if you select the group you just created it looks like that (just created "aaa"):
There was a problem hiding this comment.
Yeah, I think the .data() will store the properties in a hidden place while .attr() makes it visible on the DOM.
|
|
||
| getElementGID: function (element) { | ||
| return ($(element).closest('li').data('gid') || '').toString(); | ||
| return ($(element).closest('li').attr('data-gid') || '').toString(); |
|
@PVince81 backport the fix or only the tests that would work in stable10? |
|
@individual-it both please |
|
backport stable10 #29323 |
|
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
4 participants
Description
allow a group that is called
0to be deleted in the UIadditionally some basic UI tests to delete groups
Related Issue
#29051
Motivation and Context
see issue
How Has This Been Tested?
Types of changes
Checklist: