Page MenuHomePhabricator

Configuration of "Tab Panel" is lost when 1st tab has no panel assigned
Open, Needs TriagePublic

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.