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 directly instead of 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 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

Freeipa-devel mailing list

Reply via email to