Page MenuHomePhabricator

Configuration of "Tab Panel" is lost when 1st tab has no panel assigned
Closed, ResolvedPublic

Description

When creating a "Tab Panel" and leaving the 1st tab blank (or even any number of tabs (e.g. 1/2 blank, 3 assigned), the whole "Tab Panel" configuration will be lost when saving.

How to Reproduce?

  • Create a new "Tab Panel" on a Dashboards or create a new one
  • Configure it as shown in the screenshot below (1/2 blank, 3: "Name: Tasks", "Panel: W4 Assigned Tasks")
  • Click on Save Panel

What happens

  • The resulting "Tab Panel" will be completely empty
  • Opening the configuration dialog again shows a completely emptied configuration

What should happen

One of the following options:

  • A warning dialog tells the user to fill up the tabs starting from 1… (bad UX, but still OK)
  • The configuration is processed automatically to remove empty items and move later ones to the 1st places
  • The configuration accepts empty items, but the resulting "Tab Panel" just ignores them

Version Information

Event Timeline

eliaspro created this task.Mar 7 2017, 11:35 AM
chad added a subscriber: chad.Mar 7 2017, 2:39 PM

What are you attempting to do here or was this accidental?

In T12363#214538, @chad wrote:

What are you attempting to do here or was this accidental?

This was accidental and I just feel like this is bad UX (losing existing configuration on submit).

@epriestley as far as I can tell, empty tokenizers do not POST an input to evaluate.

We should still be able to handle this case, but I can take a look at what's happening since this is a nonstandard element and the fix is probably easy once I understand what's going on with the logic.

chad added a comment.Mar 24 2017, 1:29 PM

I think i need to set the inputs with an actual key, not just [], so [$ii]

chad added a comment.Mar 24 2017, 1:30 PM

amazing what sleep will accomplish

Based on like 10 seconds of looking at the code that's where I'd start. 💯

chad claimed this task.Mar 24 2017, 1:36 PM

Thinking about this again, what about making this hard-coded numbered list instead a list of draggable items like it's used in Menu Configuration?

chad added a comment.Mar 24 2017, 5:05 PM

I just plan to fix the bug. That's the hard part of a 2-person team with thousands of open issues. Pick and choose the battles.

chad added a comment.Mar 24 2017, 6:40 PM

Yeah I can't seem to fix this ad hoc. My guess is it's never worked to allow you to skip the first tab. If I fix the error states, it then breaks existing tab panels. So we'd need to build a migration.

diff --git a/src/applications/dashboard/customfield/PhabricatorDashboardPanelTabsCustomField.php b/src/applications/dashboard/customfield/PhabricatorDashboardPanelTabsCustomField.php
index 96566db..932cdd9 100644
--- a/src/applications/dashboard/customfield/PhabricatorDashboardPanelTabsCustomField.php
+++ b/src/applications/dashboard/customfield/PhabricatorDashboardPanelTabsCustomField.php
@@ -17,13 +17,13 @@ final class PhabricatorDashboardPanelTabsCustomField
     $names = $request->getArr($this->getFieldKey().'_name');
     $panel_ids = $request->getArr($this->getFieldKey().'_panelID');
     $panels = array();
-    foreach ($panel_ids as $panel_id) {
-      $panels[] = $panel_id[0];
+    foreach ($panel_ids as $idx => $panel_id) {
+      $panels[$idx] = $panel_id[0];
     }
     foreach ($names as $idx => $name) {
       $panel_id = idx($panels, $idx);
       if (strlen($name) && $panel_id) {
-        $value[] = array(
+        $value[$idx] = array(
           'name' => $name,
           'panelID' => $panel_id,
         );
@@ -88,21 +88,21 @@ final class PhabricatorDashboardPanelTabsCustomField
 
     $out = array();
     for ($ii = 1; $ii <= 6; $ii++) {
-      $tab = idx($value, ($ii - 1), array());
+      $tab = idx($value, $ii, array());
       $panel = idx($tab, 'panelID', null);
       $panel_id = array();
       if ($panel) {
         $panel_id[] = $panel;
       }
       $out[] = id(new AphrontFormTextControl())
-        ->setName($this->getFieldKey().'_name[]')
+        ->setName($this->getFieldKey().'_name['.$ii.']')
         ->setValue(idx($tab, 'name'))
         ->setLabel(pht('Tab %d Name', $ii));
 
       $out[] = id(new AphrontFormTokenizerControl())
         ->setUser($this->getViewer())
         ->setDatasource(new PhabricatorDashboardPanelDatasource())
-        ->setName($this->getFieldKey().'_panelID[]')
+        ->setName($this->getFieldKey().'_panelID['.$ii.']')
         ->setValue($panel_id)
         ->setLimit(1)
         ->setLabel(pht('Tab %d Panel', $ii));
chad removed chad as the assignee of this task.Apr 21 2017, 4:33 AM
chad added a subscriber: benwick.

T12708 is the same root bug. This was more engineering than I could handle.

epriestley moved this task from Backlog to Next on the Dashboards board.Sun, Mar 31, 10:00 PM

I marked this as resolved in D20384, since that rewrites this interface and solves this particular issue. The new UI is still fairly rough (thus "v1 Major Jank Edition") and has some new replacement problems, but see T13272 for followups.