Workspace indicator does not update after workspace label edited

I think this will do it.

Thanks and you're quite correct. I was about to do that first but thought it was going to override the #cache key at the top but now I see that we blow that away later.

I think it should retain the user.permissions context too.

New patch coming soon.

Yeah, I went through similar steps with similar results so I was unsure whether to leave it in. I should probably remove it.

@Eli-T, regarding the testing from #7: I think that you're not seeing any behavior change because some other part of core uses the user.permissions context in a render array, so it gets added to the page regardless of the (broken) implementation from workspaces_toolbar().

As for the issue and the patch itself, great catch! :)

I think it would be better to put the new cache tag definition under $items['workspace']['tab'] (just after #attributes), and we should also fix the problem that we override the $items['workspace'] item defined at the top of the function, by using $items['workspace'] += [ ....

Also, it would be really good to add some small test coverage for this bug in \Drupal\Tests\workspaces\Functional\WorkspaceTest.

Thanks @amateescu I can do that in the next few days if nobody else does but ...

If I put the cache tags definition under tabs rather than under workspace and then append the array rather than overwrite it, what should I do with the cache contexts definition already added to workspace? Should I leave them under workspace so we'll have contexts under workspace and tags under tabs?

Should I leave them under workspace so we'll have contexts under workspace and tags under tabs?

Yup, that's exactly what I would propose to do :)

Okay, starting again.

This is just a test which should fail.

This should pass.

This looks good to me! Just a couple minor, things I guess we can shorten directly into drupalPostForm() like attached patch / interdiff.

1. +++ b/core/modules/workspaces/tests/src/Functional/WorkspaceTest.php
   @@ -33,6 +33,13 @@ class WorkspaceTest extends BrowserTestBase {
   +  protected $workspaceAdmin;
   
   @@ -47,6 +54,15 @@ public function setUp() {
   +    $permissions = array_merge($permissions, [
   +      'access toolbar',
   +      'view own workspace',
   +      'view any workspace',
   +      'administer workspaces',
   +    ]);
   +
   +    $this->workspaceAdmin = $this->drupalCreateUser($permissions);
   

   I don't think we need to add another user to this test, we can simply add the access toolbar permission to the current list defined in the setUp() method and use $this->editor1 in the new test.

2. +++ b/core/modules/workspaces/tests/src/Functional/WorkspaceTest.php
   @@ -70,6 +86,36 @@ public function testSpecialCharacters() {
   +    // Toolbar should show correct label.
   

   show _the_ correct label :)

3. +++ b/core/modules/workspaces/tests/src/Functional/WorkspaceTest.php
   @@ -70,6 +86,36 @@ public function testSpecialCharacters() {
   +    // Toolbar should show new label.
   

   Same here, show _the_ new label.

i am uploading the patch which solves point 2 and point 3 from comment #18. Note point 1 is still remaining so i am not putting this under needs review status. I will work on that too soon if i get time in-between. Also someone can pick that remaining task by using my patch.Thanks.

I had this ready to go and just noticed #19 as I refreshed the page before uploading so I just renamed it. Sorry if that's an inappropriate thing to do.

Related to this, I wonder if the permissions are correct in workspaces.module in function workspaces_toolbar().

$current_user = \Drupal::currentUser();
if (!$current_user->hasPermission('administer workspaces')
  || !$current_user->hasPermission('view own workspace')
  || !$current_user->hasPermission('view any workspace')) {
  return $items;
}

It seems a little odd that we need both 'view own workspace' and 'view and workspace'. I wonder the intention was to need either.

@tetranz, note that the conditions are negated in workspace_toolbar() and they are checked with || (OR), which means that a user needs either one of those three permissions, not all of them at the same time :)

1. +++ b/core/modules/workspaces/tests/src/Functional/WorkspaceTest.php
   @@ -33,6 +33,13 @@ class WorkspaceTest extends BrowserTestBase {
   +   * A test user.
   +   *
   +   * @var \Drupal\user\Entity\User
   +   */
   +  protected $workspaceAdmin;
   

   This test variable is not needed anymore.

2. +++ b/core/modules/workspaces/tests/src/Functional/WorkspaceTest.php
   @@ -43,6 +50,10 @@ public function setUp() {
   +      'view any workspace',
   ...
   +      'administer workspaces',
   

   We can drop these two permissions.

working on it.

Here is the patch. Correction from #21

@amateescu Are you sure about the permissions logic? :) Maybe my brain isn't quite in gear this hour of the morning but it looks to me that:

If you don't have 'administer workspaces' or don't have 'view own workspace' or don't have 'view any workspace' then the early return happens. i.e., it's effectively an AND thanks to De Morgan. That's why I added the other user because I didn't want to pollute the other tests too much but maybe this needs to be addressed too. I think it's missing an extra outer set of parenthesis that the original author intended.

A manual test confirms that you need all three before the toolbar appears. I think @dhirendra.mishra's patch will fail because of this.

I think that logic needs to be this but ... I'm not sure if we should do that in this issue or create a separate issue.

  $current_user = \Drupal::currentUser();
  if (!($current_user->hasPermission('administer workspaces')
    || $current_user->hasPermission('view own workspace')
    || $current_user->hasPermission('view any workspace'))) {
    return $items;
  }

@tetranz, I was 100% sure, and 100% wrong apparently :) I think it's fine to use the same issue to fix that as well, mostly for laziness-related reasons, but I if you want to open a new one please go ahead.

No problem. :)

I can't do it right now so if you want do it @dhirendra.mishra before me then that's fine too although we really should have interdiffs with your patches. Hopefully that change won't break anything else but ... I guess we'll find out.

ok.working on it.Thanks.

Sorry as had some problem with interdiff. Just have changed the permission logic in workspace.module file as suggested in #24.

Please find patch below.

We need to only return nothing if user doesn't have any of the permissions, so !1 && !2 && !3:

  $current_user = \Drupal::currentUser();
  if (!$current_user->hasPermission('administer workspaces')
    && !$current_user->hasPermission('view own workspace')
    && !$current_user->hasPermission('view any workspace')) {
    return $items;
  }

Tests should now pass

Agreed, #31 accomplishes the same goal as the snippet from #24. Looks good to me now :)

Ah yes, missed that: same as suggested in #24

Oh Yes..Got it.. I had missed that. Thanks @scott_euser

Committed and pushed 03fdd37de2 to 8.7.x and f41ff929db to 8.6.x. Thanks!