[Freeipa-devel] [PATCH] 32-47 #2040, #1515 Refactor UI widgets

[Endi Sukma Dewata]

The UI seems to be working like before, so patches #32-47 are ACKed. But 
I'd rather wait until the unit tests are completely fixed before pushing.

I'm going to rebase the HBAC Test patches on top of these.

I have some comments below, but they can be addressed separately.

On 11/30/2011 10:37 AM, Petr Vobornik wrote:
>> 6. Open a self service permission, click 'select all' checkbox. The
>> update should have become enabled.
> Fixed [aci patch #43]

When resetting the page the 'select all' remains checked. This is an 
existing problem.

>> 10. In sudo.js you use apply() to call push() like this:
>>
>> spec.fields.push.apply(spec.fields, [...]);
>>
>> Why not call spec.fields.push(...) directly?
>>
> Fixed. My mindset was: i want to push an array. It would be probably OK
> if the array was already defined somewhere else. [sudo, hbac patch #45]

There are 2 more in line 242 & 249.

11. Create a permission with target 'type'. Edit it, change the type, 
the attributes will change accordingly. Then click undo/reset, the 
attributes do not revert back. This is an existing problem.

12. In IPA.checkboxes_field the 'checkbox_load' should have been 
'checkboxes_load'. The commented code is not necessary, checkboxes field 
is different because it can be empty whereas checkbox field always has a 
value (on/off).

13. The IPA.field.test_dirty() can call widget.save() directly instead 
of field.save(). It will make more sense because we're comparing the 
widget's current value with the field's original value. Also this way we 
don't have to support 2 modes in field.save(): with param and no param.

14. The IPA.rule_details_widget probably can be removed or converted 
into a field. The category radio widget and the tables probably can be 
defined directly under the section.

15. The IPA.column still depends on entity to get the column labels. If 
the table is used by a field that contains pkey of another entity, the 
field should get the entity's metadata and set the column labels when 
creating the table. If the table is used by search facet, the facet 
should supply the labels.

16. The IPA.entity_select_widget should be converted into a subclass of 
IPA.combobox_field which uses the IPA.combobox_widget. The existing 
class only contains data handling logic, it doesn't change the UI.

17. The IPA.attributes_widget probably can be converted into a subclass 
of IPA.checkboxes_field which uses the IPA.table_widget.

18. Similarly, the IPA.rights_widget can be converted into a subclass of 
IPA.checkboxes_field which uses the IPA.checkboxes_widget. In general if 
the UI doesn't change we should reuse existing widgets and we should do 
the behavior customization in the field.

19. The IPA.permission_target_widget can switch inheritance from 
different super classes (ie. collapsible or non-collapsible section) 
depending on the parameter. While this is possible in JS, the concept is 
difficult to translate to other OO framework (in case someday we want to 
change). Here's the current class hierarchy:

   section (composite widget)
    + collapsible section (details section)
      + details table section
        + permission target section             <--
        + non-collapsible details table section
          + permission target section           <--

I think the decision whether to use a collapsible or non-collapsible 
section should be separate from the permission target section. One 
solution is to use the following class hierarchy:

   section (composite widget)
    + table section
      + permission target section
      + collapsible table section <--
    + collapsible section
      + collapsible table section <--

In the details facet definition the permission target section should be 
specified as a member widget (not a subclass) of collapsible section. In 
the dialog box the permission target section should be used directly.

For other widgets in the details facet, they can be put under a 
collapsible table section. The class can be implemented either as a 
subclass of table section or collapsible section, whichever is easier, 
but it doesn't mean it can switch inheritance.

-- 
Endi S. Dewata