[racktables-users] Re: My first usability patch - extensive

On Mon, Jun 9, 2008 at 2:26 PM, Denis Ovsienko <pilot@xxxxxxxxxx> wrote:
> Hello, all.
>
> Jonathan, thank you for the effort. Although you seem to understand the
> coding style, I would like to provide some feedback before you go too
> far. You are adding more accessor functions for RackRow and hardware
> model dictionary chapters. The current codebase has evolved from
> multiple simple tables and even more numerous simple functions to one
> single "dictionary" structure and several uniform functions to access
> it. This made subsequent improvements easier for development. What you
> have done is closely related to a planned improvement, which has been
> filed as http://racktables.org/trac/ticket/151 I would be glad to see
> the latter ticket closed, the current editor is a bit annoying. It
> does its work though.

I completely agree on limiting the functions that handle Dictionary / Chapter
manipulation.  I used the current functions to the best of there ability.
Unfortunately, for the pages/tabs that I am trying to return to I
would like to see more flexibility in the URL (to land back on the
correct item).
To help with this I thought maybe a function to generate the return URL for
ophandlers would make sense.  That way the constant repeat of
"${root}?page=${pageno}&tab=${tabno}&....." could be removed and an
additional $_REQUEST variable to contain items to append to the resulting
URL.  I have included another patch below that shows implements this idea.
If it looks good to you, then I can fix all of ophandlers.php.

>> Here is a brief list of the changes I made:
>> ---------------------------------------------------------------------------
>>
>> Rackspace Page
>>   * Added printing of Messages / Errors
>>   * "Add more" tab to allow adding multiple Rack Rows
>
> See above.
>
>>
>> Row Page
>>   * Add "Delete" tab to Rows with no racks, with confirmation page
>>
>> Rack Page
>>   * Add "Delete" tab to Rack with no objects mounted, with
>> confirmation page
>
> You forgot to wipe the RackHistory records along with rackspace history
> records.

Thanks, I will check into that.

>>
>> Configure Page
>>   * Hardware Objects
>>     * Add/Delete/Update Hardware Categories ("HW Type" Dictionary
>> Attributes)
>>     * Add/Delete/Update Items in above mentioned Categories
>>
>> Hardware Objects Page
>>   * New page found under Configuration page
>>   * Makes managing Hardware categories (Routers, Network Switches,
>> etc) MUCH easier and more automated
>>
>> General Changes
>>   * Visual adjustment: Make all racks align to the "floor" with each
>> other on Rack / Rackspace pages. I didn't like my 40U racks
>> "floating" between the 42U racks
>
> I think, that the best alignment depends if the racks stand on the floor
> or are attached to walls. This probably sooner deserves a new column in
> the Rack SQL table, than changing one fixed alignment to another.

I think that this would be better suited for a variable, but for the time being,
I just want all of the rack names aligned.  The current implementation has the
valign=center which makes it more difficult to read the rack names.  I will move
this to my pet-peeve.patch for now :)

>>   * Moved function "delRow" from inc/functions.php to
>> inc/ophandlers.php Rewrote delRow breaking it out to many functions
>>   * Moved function "delRack" from inc/functions.php to
>> inc/ophandlers.php Rewrote delRack breaking it out to many functions
>
> Oh, I see now. It was probably the oldest code in the whole project,
> somehow it managed to survive in a couple of unused functions. I'm
> half-merging the change, i.e. deleting the old implementation from
> functions.php. Update your working copy.
>
> There's a couple of more challenging key features, which I would prefer
> to concentrate on. One is replacing the current permissions framework
> with a more natural text-based ruleset, which would use the tags system
> introduced by 0.15 series. Another one is a universal 2D-mapping
> approach, which would describe positioning of racks in rooms, units in
> racks and modules in modular devices. If you would like to help with
> that, let me know.

I would be happy to work on the nested 2D placement code.  Support for
items such as blade servers is definitely needed, as well as the ability
to order racks in a room.  If you have any ideas on what you want to see,
send them to me.

I do understand that you have features that you would really like to get done.
From my perspective, and that of my users, an interface that simplifies updating
hardware / software / etc types is vital.  I can live with the racks
not in the right
order, but having to update objects because people keep using "unknown/other"
for the hardware is beginning to become cumbersome.  I am willing to work more
towards a general way of editing the dictionary, but at some level the interface
needs to speak to what is actually being entered, and the desired fields.  While
I don't believe that my current implementation is the end of the development for
this functionality, I do see it as a starting point, which is why it
was submitted for
input.

Thanks,

-Jonathan

Here is the patch that I mentioned above (svn diff, 1957):

Index: inc/ophandlers.php
===================================================================
--- inc/ophandlers.php  (revision 1957)
+++ inc/ophandlers.php  (working copy)
@@ -81,6 +81,35 @@
        }
 }

+/*
+ *   Author: Jonathan Thurman
+ * Modified: 2008-06-09
+ * Generate the return url for ophandlers, allows easier change to url
+ * - Allow function to over ride tab and page
+ * - Look for 'append_url' variable and append it
+ *
+ * Usage of append_url: append_url = urlencode("arg1=test|arg2=test2");
+ *   result = "${base_url}&arg1=test&arg2=test2"
+ */
+function generateReturnURL ($tab = "", $page = "")
+ {
+       global $root, $pageno, $tabno;
+
+       // Allow over ride of the global page / tab
+       $base_url = "${root}?page=" .
+               (empty($page) ? $pageno : $page) . "&tab=" .
+               (empty($tab) ? $tabno : $tab);
+       if (isset($_REQUEST['append_url']))
+       {
+               $str = urldecode($_REQUEST['append_url']);
+               $args = explode('|', $str);
+               foreach ($args as $a)
+                       $base_url .= "&" . $a;
+       }
+               
+       return $base_url;
+}
+
 function addPortForwarding ()
 {
        global $root, $pageno, $tabno;
@@ -688,77 +717,63 @@
                "=" . urlencode ("${error_count} failures and ${success_count}
successfull changes.");
 }

-function deleteDictWord ()
-{
-       global $root, $pageno, $tabno;
-       return
-               "${root}?page=${pageno}&tab=${tabno}&" .
-               "error=" . urlencode ('Dragon ate this word!');
-}
-
 function updateDictionary ()
 {
-       global $root, $pageno, $tabno;
        assertUIntArg ('chapter_no', __FUNCTION__);
        assertUIntArg ('dict_key', __FUNCTION__);
        assertStringArg ('dict_value', __FUNCTION__);
        if (commitUpdateDictionary ($_REQUEST['chapter_no'],
$_REQUEST['dict_key'], $_REQUEST['dict_value']) === TRUE)
-               return "${root}?page=${pageno}&tab=${tabno}&message=" . 
urlencode
('Update succeeded.');
+               return generateReturnURL() . "&message=" . urlencode ('Update
succeeded.');   // JT: 2008-06-10
        else
-               return "${root}?page=${pageno}&tab=${tabno}&error=" . urlencode
("Update failed!");
+               return generateReturnURL() . "&error=" . urlencode ("Update
failed!");              // JT: 2008-06-10
 }

 function supplementDictionary ()
 {
-       global $root, $pageno, $tabno;
        assertUIntArg ('chapter_no', __FUNCTION__);
        assertStringArg ('dict_value', __FUNCTION__);
        if (commitSupplementDictionary ($_REQUEST['chapter_no'],
$_REQUEST['dict_value']) === TRUE)
-               return "${root}?page=${pageno}&tab=${tabno}&message=" . 
urlencode
('Supplement succeeded.');
+               return generateReturnURL() . "&message=" . urlencode 
('Supplement
succeeded.');   // JT: 2008-06-10
        else
-               return "${root}?page=${pageno}&tab=${tabno}&error=" . urlencode
("Supplement failed!");
+               return generateReturnURL() . "&error=" . urlencode ("Supplement
failed!");      // JT: 2008-06-10
 }

 function reduceDictionary ()
 {
-       global $root, $pageno, $tabno;
        assertUIntArg ('chapter_no', __FUNCTION__);
        assertUIntArg ('dict_key', __FUNCTION__);
        if (commitReduceDictionary ($_REQUEST['chapter_no'],
$_REQUEST['dict_key']) === TRUE)
-               return "${root}?page=${pageno}&tab=${tabno}&message=" . 
urlencode
('Reduction succeeded.');
+               return generateReturnURL() . "&message=" . urlencode ('Reduction
succeeded.');   // JT: 2008-06-10
        else
-               return "${root}?page=${pageno}&tab=${tabno}&error=" . urlencode
("Reduction failed!");
+               return generateReturnURL() . "&error=" . urlencode ("Reduction
failed!");      // JT: 2008-06-10
 }

 function addChapter ()
 {
-       global $root, $pageno, $tabno;
        assertStringArg ('chapter_name', __FUNCTION__);
        if (commitAddChapter ($_REQUEST['chapter_name']) === TRUE)
-               return "${root}?page=${pageno}&tab=${tabno}&message=" . 
urlencode
('Chapter was added.');
+               return generateReturnURL() . "&message=" . urlencode ('Chapter 
was
added.');       // JT: 2008-06-10
        else
-               return "${root}?page=${pageno}&tab=${tabno}&error=" . urlencode
('Error adding chapter.');
+               return generateReturnURL() . "&error=" . urlencode ('Error 
adding
chapter.');     // JT: 2008-06-10
 }

 function updateChapter ()
 {
-       global $root, $pageno, $tabno;
        assertUIntArg ('chapter_no', __FUNCTION__);
        assertStringArg ('chapter_name', __FUNCTION__);
        if (commitUpdateChapter ($_REQUEST['chapter_no'],
$_REQUEST['chapter_name']) === TRUE)
-               return "${root}?page=${pageno}&tab=${tabno}&message=" . 
urlencode
('Chapter was updated.');
+               return generateReturnURL() . "&message=" . urlencode ('Chapter 
was
updated.');     // JT: 2008-06-10
        else
-               return "${root}?page=${pageno}&tab=${tabno}&error=" . urlencode
('Error updating chapter.');
+               return generateReturnURL() . "&error=" . urlencode ('Error 
updating
chapter.');     // JT: 2008-06-10
 }

 function delChapter ()
 {
-       global $root, $pageno, $tabno;
        assertUIntArg ('chapter_no', __FUNCTION__);
        if (commitDeleteChapter ($_REQUEST['chapter_no']))
-               return "${root}?page=${pageno}&tab=${tabno}&message=" . 
urlencode
('Chapter was deleted.');
+               return generateReturnURL() . "&message=" . urlencode ('Chapter 
was
deleted.');     // JT: 2008-06-10
        else
-               return "${root}?page=${pageno}&tab=${tabno}&error=" . urlencode
('Error deleting chapter.');
+               return generateReturnURL() . "&error=" . urlencode ('Error 
deleting
chapter.');     // JT: 2008-06-10
 }

 function changeAttribute ()

Other related posts: