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 ()