Link to each dashboard's canonical URL somewhere

Interesting! After manually flushing caches for the first time, a "Dashboards" link appeared in my Toolbar, so I've deleted that part from the issue summary.

I've made a patch to link the dashboard label to the canonical URL from entity.dashboard.collection.

Reviews welcome!

In \Drupal\dashboard\DashboardListBuilder, I was forced to change the name of the label column to link in both DashboardListBuilder::buildHeader() and DashboardListBuilder::buildRow(), because I discovered that \Drupal\Core\Entity\DraggableListBuilderTrait forces a column named label (if it exists) to be plain text with the following code in DraggableListBuilderTrait::buildForm():

      if (isset($row['label'])) {
        $row['label'] = ['#plain_text' => $row['label']];
      }

Discuss if we should add a "View" link to the Primary Tabs that will take the user to view the page.

I'm not sure where this is recorded, but it looks like the "View" link is missing from dashboard.links.task.yml because the page at /admin/dashboard uses the primary links to allow users to switch between the dashboards they are allowed to see — and when you add it, you get tabs for other dashboards and the current dashboard all mixed together, which is confusing, and likely a bad idea.

Maybe a link from /admin/structure/dashboard is sufficient? But I'd be interested to hear feedback from the maintainers, who have put a lot more thought/work into this module than I have.

Updating the title

I still need to add tests for this change.

🐛 Wrong redirect after Dashboard layout edition Active would definitely help here. Once you save, you are redirected to the dashboard canonical url instead of the listing.

The toolbar caching is definitely an issue, can you create a separate one? We've been mostly focusing on the new navigation.
The idea is that the toolbar/new navigation "Dashboard" link gives you the entry point to all the dashboards.

An option could be having on DashboardListBuilder:

  public function buildForm(array $form, FormStateInterface $form_state) {
    $form = parent::buildForm($form, $form_state);
    foreach ($this->entities as $entity) {
      $form[$this->entitiesKey][$entity->id()]['label'] = [
        '#type' => 'link',
        '#title' => $entity->label(),
        '#url' => $entity->toUrl('canonical'),
      ];
    }
    return $form;
  }

Hi!

Removing the "View" local task was made on purpose because having the ability to create or edit a dashboard does not give the access to it.
That's why the "Preview" local task was created, ensuring that users that can create/edit the dashboard can get access to preview, even if they don't have access to it from /admin/dashboard local tasks.

Regarding the penyaskito's approach, that could be an option, but that link would point to a 404 error if the current user does not have access to the dashboard. We might just only make links to those dashboards the current user has access to, but it could be confusing.

I don't have a clear answer to this request that would make everybody happy.

@penyaskito, @plopesc, thank you for the quick replies; I apologize for my own delay in getting back to you!

@penyaskito wrote,

🐛 Wrong redirect after Dashboard layout edition Active would definitely help here. Once you save, you are redirected to the dashboard canonical url instead of the listing.

I agree, this would help, as it would have cleared up some of my own misunderstanding while I was testing.

@penyaskito wrote,

The toolbar caching is definitely an issue, can you create a separate one?

Okay! I will edit this message and post a link here when I've done so.

@penyaskito wrote,

An option could be having on DashboardListBuilder:

  public function buildForm(array $form, FormStateInterface $form_state) {
    $form = parent::buildForm($form, $form_state);
    foreach ($this->entities as $entity) {
      $form[$this->entitiesKey][$entity->id()]['label'] = [
        '#type' => 'link',
        '#title' => $entity->label(),
        '#url' => $entity->toUrl('canonical'),
      ];
    }
    return $form;
  }

I like this code better; I'll change my merge request shortly!

I'll reply to @plopesc's message soon.

@plopesc wrote,

Removing the "View" local task was made on purpose because having the ability to create or edit a dashboard does not give the access to it.
That's why the "Preview" local task was created, ensuring that users that can create/edit the dashboard can get access to preview, even if they don't have access to it from /admin/dashboard local tasks.

Regarding the penyaskito's approach, that could be an option, but that link would point to a 404 error if the current user does not have access to the dashboard. We might just only make links to those dashboards the current user has access to, but it could be confusing.

Interesting... I hadn't considered that specific scenario.

@penyaskito's buildForm() code alters the table row to add the link... would it be an acceptable compromise to check $entity->toUrl('canonical')->access() before transforming the label into a link, like this...?

public function buildForm(array $form, FormStateInterface $form_state) {
    $form = parent::buildForm($form, $form_state);
    foreach ($this->entities as $entity) {
      $dashboardUrl = $entity->toUrl('canonical');
      if ($dashboardUrl->access()) {
        $form[$this->entitiesKey][$entity->id()]['label'] = [
          '#type' => 'link',
          '#title' => $entity->label(),
          '#url' => $dashboardUrl,
        ];
      }
    }
    return $form;
  }

(moving back to "needs review" to get feedback)

(We can check in tugboat for this MR: https://mr59-be4pskurppwvqfqo7pjfutudyzyllyze.tugboatqa.com/en/admin/str..., login as admin/admin)

I think it makes sense to just wrap in a $dashboard->access('view') check.

It could look weird if you don't have access to a given dashboard that won't be linked, but I guess we can live with that.

Just found that we don't have any test for the listing, so might be a good opportunity to add one.