After #1826602: Allow all configuration entities to be enabled/disabled it has become apparent that the status is not updated correctly when views objects are loaded from the user tempstore.

Here is the fix, but we need tests.

I'm on it. I will write some tests in the morning.

CommentFileSizeAuthor
#9 vdc-1909878-8.patch3.8 KBtim.plunkett
#8 1909878-8.patch3.56 KBdamiankloip
#4 1909878-4.patch3.52 KBdamiankloip
#4 interdiff.txt1.11 KBdamiankloip
#2 1909878-2-tests-only.patch2.96 KBdamiankloip
#2 1909878-2.patch3.52 KBdamiankloip
d8.views_ui_cache_load_status.patch577 bytesdamiankloip

Comments

damiankloip’s picture

damiankloip commented
Issue tags: +Needs tests
damiankloip’s picture

damiankloip commented
Status: Active » Needs review
Issue tags: -Needs tests
StatusFileSize
new 1909878-2.patch3.52 KB
new 1909878-2-tests-only.patch2.96 KB
dawehner’s picture

dawehner
Primary language German
commented

Awesome!

+++ b/core/modules/views/lib/Drupal/views/Tests/UI/CachedDataUITest.phpundefined
@@ -0,0 +1,75 @@
+    $this->assertFalse($view_cache->locked, 'The view is not locked.');

We could refactor that in the future to use unit tests, as they are just function calls.

+++ b/core/modules/views/lib/Drupal/views/Tests/UI/CachedDataUITest.phpundefined
@@ -0,0 +1,75 @@
+    $this->assertText('* All changes are stored temporarily. Click Save to make your changes permanent. Click Cancel to discard your changes.', 'The view has been changed.');
...
+    $this->assertText('The view test_view has been saved.');

I guess a t() function would help here.

+++ b/core/modules/views/lib/Drupal/views/Tests/UI/CachedDataUITest.phpundefined
@@ -0,0 +1,75 @@
+    $this->assertTrue($view_cache->locked, 'The vierw is locked.');

vierwrrr, harharhar!

damiankloip’s picture

damiankloip commented
StatusFileSize
new interdiff.txt1.11 KB
new 1909878-4.patch3.52 KB

Nice, thanks for the review!

I'm not sure about unit tests in the future, as I think we should keep testing this based on UI events, but not sure on that.

I made those other changes, see interdiff.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

Awesome!

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Issue tags: -VDC

#4: 1909878-4.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work
Issue tags: +VDC

The last submitted patch, 1909878-4.patch, failed testing.

damiankloip’s picture

damiankloip commented
Status: Needs work » Needs review
StatusFileSize
new 1909878-8.patch3.56 KB

Rerolled to use the new stuff from the views UI routes patch.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
StatusFileSize
new vdc-1909878-8.patch3.8 KB

Rerolled for #1904854: Convert Views UI to routes

damiankloip’s picture

damiankloip commented

x post? I think #9 might fail. Didn't you remove views_ui_cache_load? :)

Status: Needs review » Needs work

The last submitted patch, vdc-1909878-8.patch, failed testing.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Needs work » Reviewed & tested by the community

Ah yes, I didn't see that in the test coverage. Your patch is clearly better :)

dawehner’s picture

dawehner
Primary language German
commented

#9: vdc-1909878-8.patch queued for re-testing.

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Committed #8 to 8.x. Thanks.

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

Anonymous’s picture

(not verified) commented
Issue summary: View changes

Updated issue summary.