Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
If when editing a view, you rearrange a list of fields and then apply the changes, all the items in the list will be removed. This happens to both the list of fields and the items in the sort criteria.
Just to add some extra data for troubleshooting:
You do not have to actually rearrange any of the items. Just clicking the Apply button will remove the items.
Canceling the dialog will not remove the items.
Comment | File | Size | Author |
---|---|---|---|
#21 | 2002506-Cannot-Rearrange-Views-Fields-21.patch | 3.55 KB | pcambra |
#21 | interdiff.txt | 2.18 KB | pcambra |
#18 | 2002506-Cannot-Rearrange-Views-Fields-18.patch | 3.74 KB | pcambra |
#16 | 2002506-Cannot-Rearrange-Views-Fields-16-fail.patch | 2.83 KB | pcambra |
#16 | 2002506-Cannot-Rearrange-Views-Fields-16.patch | 3.63 KB | pcambra |
Comments
Comment #1
pcambraTagging
Comment #2
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #3
oadaeh CreditAttribution: oadaeh commentedI forgot to add earlier that the same functionality in the filters list is not broken (at least not in this way).
Comment #4
Anonymous (not verified) CreditAttribution: Anonymous commentedThe rearrange handler for foreach'ing over $form_state['values'] rather than $form_state['values']['fields'].
Comment #5
Psikik CreditAttribution: Psikik commentedI experienced this issue also and the patch applies cleanly and fixes the bug.
Comment #6
oadaeh CreditAttribution: oadaeh commentedThe patch in #4 fixes the problem for me, too. It also seems to be correctly formed.
I'm marking it as RTBC, but we may want to wait for the testbot (just in case).
Comment #8
Anonymous (not verified) CreditAttribution: Anonymous commented#4: 2002506-Cannot-Rearrange-Views-Fields-004.patch queued for re-testing.
Comment #9
Anonymous (not verified) CreditAttribution: Anonymous commentedResetting the issue to RTBC since that is what it was before the testbot went mental.
Comment #10
Dries CreditAttribution: Dries commentedCan we have a test for this please? Thanks!
Comment #11
longwaveThis was apparently mistakenly committed in http://drupalcode.org/project/drupal.git/commitdiff/db2df2d
Comment #12
Dries CreditAttribution: Dries commentedRerolled this patch.
Comment #13
Anonymous (not verified) CreditAttribution: Anonymous commentedAdded tests.
Comment #14
damiankloip CreditAttribution: damiankloip commentedNice start!
Can we add an '@see \Drupal\views_ui\Form\Ajax\Rearrange' here too.
We don't use private, these should all be protected. Also, the properties need docblocks.
We generally don't use other modules in tests unless we really need to. We try to use views_test_data for as much of this as possible. So I think we should convert to use that maybe.
Rearrange fields? Maybe we should also test the reordering of sorts too.
This could just be 'Creates a random view' but I think it should also say via the wizard/ui too. Also, pretty much the same thing is used in DisplayTest, maybe we should look at moving a form of this method into UiTestBase? Also, we could just use a new test view instead, then there would be no need to create in the test, as we aren't testing that here.
'Tests'
These don't really need to be static properties? If they do, we should use the static keyword and not the actual class name. Seems like a regular property would be fine though.
Needs newline at EOF.
Comment #15
pcambraHere's a rework of the field test, I've removed the new view to rely on the test_view one, I think the only thing that we have left here is decide if we want the sort test or not... (shall we need to add then testing reorder on filters, area handlers, arguments...?)
Comment #16
pcambraIt's better if I give the testbot something to test :P, renaming the class file.
Comment #17
dawehnerThis is basically RTBC. Thank you!
Should also be Tests instead of tests
Nice!
Let's document the two parameters.
Just remove the starting '/' as you never know whether drupal will be installed in a folder.
Comment #18
pcambraFixed comments in #17
Comment #19
dawehnerThank you!
Comment #20
alexpottCan we combine these into one test method... each test necessitates a full re-install of drupal and this seems overkill.
Comment #21
pcambraHere we go :)
Comment #22
tstoecklerThe interdiff definitely fixes #20 and it was RTBC before, so let's do this.
Comment #23
alexpottCommitted f7ded17 and pushed to 8.x. Thanks!