It's broken for non-AJAX callbacks as well, so that will be testable.
rearrange1.pngrearrange2.pngrearrange3.png

Files: 
CommentFileSizeAuthor
#12 1904932-12.patch2.67 KBYesCT
PASSED: [[SimpleTest]]: [MySQL] 55,570 pass(es).
[ View ]
#12 interdiff-11-12.txt960 bytesYesCT
#11 1904932-11.patch2.68 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 55,520 pass(es).
[ View ]
#11 interdiff-1904932-11.txt3.23 KBdamiankloip
#8 1904932-8.patch3.77 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 53,864 pass(es).
[ View ]
#8 interdiff.txt1.89 KBdamiankloip
#2 drupal-1904932-2.patch3.68 KBYesCT
FAILED: [[SimpleTest]]: [MySQL] 48,716 pass(es), 2 fail(s), and 3 exception(s).
[ View ]
#2 interdiff-1-2.txt625 bytesYesCT
#1 drupal-1904932-1.patch3.69 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 48,725 pass(es).
[ View ]
rearrange3.png12.69 KBtim.plunkett
rearrange2.png45.05 KBtim.plunkett
rearrange1.png25.16 KBtim.plunkett

Comments

dawehner’s picture

Status:Active» Needs review
StatusFileSize
new3.69 KB
PASSED: [[SimpleTest]]: [MySQL] 48,725 pass(es).
[ View ]
YesCT’s picture

Status:Needs review» Needs work
StatusFileSize
new625 bytes
new3.68 KB
FAILED: [[SimpleTest]]: [MySQL] 48,716 pass(es), 2 fail(s), and 3 exception(s).
[ View ]

manually tested. And the js error is gone, but it jumps me out of the views overlay and wizard and to the front page.

tried to improve grammar of the comment. See if it still says the correct meaning of what you wanted it to say.

YesCT’s picture

Status:Needs work» Needs review
YesCT’s picture

Status:Needs review» Needs work
dawehner’s picture

manually tested. And the js error is gone, but it jumps me out of the views overlay and wizard and to the front page.

So we probably want to set this link to use an empty fragment, so it doesn't do anything for itself, without the javascript?

YesCT’s picture

@dawehner How do we do that?

Also, this is "needs tests"
what kind of test should we add? webtest?

dawehner’s picture

Well, the removing just works on the javascript so I don't see how we should be able to test this?
The error mentioned by tim happens if you click on the link but the javascript fails.

damiankloip’s picture

Status:Needs work» Needs review
Issue tags:-Needs tests
StatusFileSize
new1.89 KB
new3.77 KB
PASSED: [[SimpleTest]]: [MySQL] 53,864 pass(es).
[ View ]

Gave this a quick tidy up, now it matches alot of the other js code (I think :)). Also tested, and confirm the fix is good.

Removing needs tests too; Javascript is rubbish, and hard to test ;)

dawehner’s picture

+++ b/core/modules/views/views_ui/js/views-admin.jsundefined
@@ -1098,6 +1098,34 @@ Drupal.viewsUi.resizeModal = function (e, no_shrink) {
+  /**
+   * Handle handler deletion by looking for the hidden checkbox and hiding the
+   * row.
+   */
...
+  /**
+   * Handle display deletion by looking for the hidden checkbox and hiding the
+   * row.
+   */

These aren't method documentations so should we use "//" instead?

olli’s picture

Status:Needs review» Needs work
+++ b/core/modules/views/views_ui/js/views-admin.js
@@ -1098,6 +1098,34 @@ Drupal.viewsUi.resizeModal = function (e, no_shrink) {
+Drupal.behaviors.handlerRemoveLink = {};

.handlerRemoveLink -> .viewsUiHandlerRemoveLink

+++ b/core/modules/views/views_ui/js/views-admin.js
@@ -1098,6 +1098,34 @@ Drupal.viewsUi.resizeModal = function (e, no_shrink) {
+    $('#views-row-' + id).hide();
+    $('#views-removed-' + id).attr('checked', true);

These could use the context param too.

+++ b/core/modules/views/views_ui/js/views-admin.js
@@ -1098,6 +1098,34 @@ Drupal.viewsUi.resizeModal = function (e, no_shrink) {
+  $('a.display-remove-link', context).addClass('display-processed').click(function(event) {

Why not .once() instead of .addClass()?

+++ b/core/modules/views/views_ui/js/views-admin.js
@@ -1098,6 +1098,34 @@ Drupal.viewsUi.resizeModal = function (e, no_shrink) {
+      var id = $(this).attr('id').replace('display-remove-link-', '');
+      $('#display-row-' + id, context).hide();
+      $('#display-removed-' + id, context).attr('checked', true);
+      event.preventDefault();
+  });

Indentation.

damiankloip’s picture

Status:Needs work» Needs review
StatusFileSize
new3.23 KB
new2.68 KB
PASSED: [[SimpleTest]]: [MySQL] 55,520 pass(es).
[ View ]

Rerolled and added in feedback from #9 and #10, thanks. Its been a while!

YesCT’s picture

StatusFileSize
new960 bytes
new2.67 KB
PASSED: [[SimpleTest]]: [MySQL] 55,570 pass(es).
[ View ]
+++ b/core/modules/views_ui/js/views-admin.jsundefined
@@ -1098,6 +1098,32 @@ Drupal.viewsUi.resizeModal = function (e, no_shrink) {
+Drupal.behaviors.viewsUIhandlerRemoveLink = {};
+Drupal.behaviors.viewsUIhandlerRemoveLink.attach = function(context) {

In this js file, UIwhatever is more using the pattern: UiWhatever.

+++ b/core/modules/views_ui/js/views-admin.jsundefined
@@ -1098,6 +1098,32 @@ Drupal.viewsUi.resizeModal = function (e, no_shrink) {
+  /**
+   * Handle handler deletion by looking for the hidden checkbox and hiding the
+   * row.
+   */
...
+  // Handle display deletion by looking for the hidden checkbox and hiding the
+  // row.

missed one. :)

YesCT’s picture

verified that the bug still exists without the patch.

manually tested with the patch:
Works great, stays in views ui, and I can save. Everything fine.

Next steps:
a code review (I should not rtbc as I did the most recent patch).

======

Actually, with and without this patch, rearranging the sort criteria or the no results behavior... results in all of those being deleted instead of rearranged. Rearranging the filter criteria works fine though. I dont see a issue for rearranging sort criteria bug (yet).

Should not stop this issue as it is a pre-existing bug.

YesCT’s picture

Status:Needs review» Reviewed & tested by the community

I talked to @damiankloip in irc.
rtbc!

tim.plunkett’s picture

RTBC +1

alexpott’s picture

Status:Reviewed & tested by the community» Fixed

Committed f140bce and pushed to 8.x. Thanks!

I guess we need a new issue for the bug mentioned by @YesCT in #13

damiankloip’s picture

I think #1929070: Refactor views_ui_rearrange_filter_form to remove theme function for table rendering is related to that, and will possibly/probably/maybe fix what is mentioned by YesCT.

Automatically closed -- issue fixed for 2 weeks with no activity.