In addition, we have no test coverage for the handling of disabled views, especially with the interaction of stale temp stores.

And, we have code that checks to see if a display is disabled, but that ignores the View's status in ViewEditFormController::getDisplayDetails()

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

We should probably asked the UI team about a general pattern in core?

tim.plunkett’s picture

Issue tags: +Usability

Tagging.

tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned

Maybe some from the usability team can provide guidance.

damiankloip’s picture

Status: Active » Needs review
FileSize
2.54 KB

To start with I think it would make sense to include the status of the view in the actual edit form wrapper? I have also started adding tests with a basic addition to the page display test, to make sure that a disabled view returns a not found.

dawehner’s picture

+++ b/core/modules/views_ui/lib/Drupal/views_ui/ViewEditFormController.phpundefined
@@ -82,7 +82,9 @@ public function form(array $form, array &$form_state, EntityInterface $view) {
+    $view_status = $view->status() ? 'enabled' : 'disabled';
+    $form['#prefix'] .= '<div class="views-edit-view views-admin ' . $view_status . ' clearfix">';

I'm wondering whether 'enabled' is specific enough for a css class?

damiankloip’s picture

Not sure, you have the other classes too though - so I think that's OK. See what people think I guess :)

dawehner’s picture

Yeah we can change it later, if needed. Do you think we should add a test for that bit?

Bojhan’s picture

Can this get a screen?

dawehner’s picture

Well, we still need some css to show anything :)

damiankloip’s picture

I'm going to ask a ux person about this today.

damiankloip’s picture

Added some tests for the actual class too.

Do you think we should get this in now? The logic will then be completed, and another issue can just deal with the UX/theme side of this? This seems like a sensible step.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Usability, -Needs tests

I agree that a design discussion should happen in a separate issue, maybe even one each for Seven and Bartik.
At the least, this will allow contrib to use this class.
And it allows us to add test coverage.

Bojhan’s picture

Issue tags: +Usability, +Needs tests

Sure, I see no reason not just to add it here though :P. It seems like we could do it quite easily.

Bojhan’s picture

Issue tags: -Usability, -Needs tests

whoops

tim.plunkett’s picture

Add what here? No one has proposed any actual UI improvements. I can implement any design you throw at me, but I am not qualified to invent one :)

damiankloip’s picture

Yeah, we are not doing any cosmetic changes here now.

Bojhan’s picture

Ok, let me know when the followup is created and I will whip up a mockup

damiankloip’s picture

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/views/lib/Drupal/views/Tests/Plugin/DisplayPageTest.phpundefined
@@ -46,6 +46,14 @@ public function testPageResponses() {
+    // Test accessing a page for a disabled view.
+    $this->drupalGet('test_page_display_disabled');
+    $this->assertResponse(200);
+    // Disable the view, rebuild menu, and request the page again.
+    $view->storage->disable()->save();
+    $this->drupalGet('test_page_display_disabled');
+    $this->assertResponse(404);

The comments here make no sense... first you access a disabled view and then you disable the view and test again...

I think what you mean is Test accessing a disabled page for a view.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
3.42 KB

Rerolled, and reworked the DispayPageTest assertions to fit with Daniels shiny new route based tests.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

That is better now!

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

The last submitted patch, 1921748-20.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
Issue tags: +VDC

#20: 1921748-20.patch queued for re-testing.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

This time it passed

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 96c82a2 and pushed to 8.x. Thanks!

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