[Freeipa-devel] [PATCH] 028 Code cleanup of HBAC, Sudo rules

[Petr Vobornik]

I'll rebase this patch on recent patches (which will be soon pushed).

This effort is doing too many things, I'll split the patch into several.


On 10/25/2011 07:57 PM, Endi Sukma Dewata wrote:
> On 10/25/2011 7:49 AM, Petr Vobornik wrote:
>>> 2. The set_facet() is added to widget. I don't think we want to make
>>> widget dependent on facet. The facet so far is only used by
>>> IPA.sudo.enable_widget. In IPA.sudo.options_section the facet object is
>>> passed as a parameter in spec.
>
>> Are you saying that dependency on facet in ALL widgets is bad and only
>> in a few is good, or in any is bad?
>
> In general less dependency is better. More dependency will limit its
> usage. This is inline with the goal to make purely HTML widget.
>
> But if a widget is meant to be used only with a facet then it's
> certainly ok to make it depends on facet. However, the rest should not
> become unnecessarily dependent on facet too.
>> I think it doesn't matter if all is dependant when one is dependant.
>>
>> Anyway currently all association table widgets are dependant
>> (association.js:386).
>
> Let's use this as an example. The association table explicitly checks if
> the facet is dirty and asks the user whether to update/reset/cancel.
> Suppose we want to use this inside a dialog, it will still check whether
> the current facet is dirty instead of the dialog itself, which may not
> be what we want.

Removed. Using instead: that.entity.get_facet() and 
that.entity.get_primary_key().

But still I think it would be better to be able to get container 
(facet/dialog) for a widget. As you wrote, that.entity.get_facet() may 
not always be what we want.

>
>> Btw similar topic could be: "How to get current entity's pkey?'.
>> Dependancy on facet.get_primary_key() or
>> IPA.nav.get_state(that.entity.name+'-pkey') seems wrong too.
>
> Yes. Ideally we should have a path (REST-style URL) instead of the
> current parameter-based URL. We should get the pkey from that path.

 From dependency point of view, widgets would be still dependant on the 
implementation of navigation (IMHO bad).

But it seems that using entity.get_primary key partially solves the problem.

Maybe it would be better if navigation would inject pkey to entity. 
Entity wouldn't be that much dependant on navigation implementation.

>>> 3. When updating sudo rule, in the original code the rule is
>>> enabled/disabled after all the modifications are done. In the new code
>>> it's reversed. I'm not sure the importance of the execution order for
>>> this particular situation, but suppose it is, is there a way to maintain
>>> the original order?
>>
>> I was thinking about a command or widget priority. The 'mod' command
>> would have some default priority or it would get it from facet. The
>> commands would be sorted by priority before adding them to batch. This
>> way we can set exact order in facet update.
>
> It's possible, but managing priority could be problematic if they are
> spread out, because we have to know all existing priorities before we
> can add a new one.

The priority could be part of update info. As there is a field_info we 
can create a command_info: { command, priority }.
 > The original code uses an ordered list of commands.
Before adding commands to batch_command, array of command_info objects 
would joined with 'mod' command with default priority and then sorted by 
priority.

If the priority was set on each widget, priority management will be on 
facet level, which may be fine. There can be some corner case like 
dynamic change of priority.

>> The order which is implemented now is reflecting the order in HBAC
>> details facet - there were errors when 'mod' was executed before
>> clearing associations.
>
> Right, so for certain things the order is important.
>
>>> 4. The IPA.header_widget is not really a widget, it's actually part of
>>> layout. In the current implementation a widget always corresponds to an
>>> attribute, so it will be loaded, saved, etc. Since there is no attribute
>>> name matching the header name currently this is not a problem, but it
>>> can pollute the namespace.
>>
>> This is part of widget nesting topic. In this manner composite_widget
>> isn't a proper widget too (it can correspond to several or none
>> attribute).
>>
>> Purely html rendering widgets can be separated from attribute widgets,
>> but it would be nice to have them because they can provide easier form
>> composing. Without them it would be required to override the create
>> method (in facet or composite_widget/section) which is sometimes better
>> but often it creates only code duplication with little added logic.
>
> One other solution is to split widgets into non-visual fields and purely
> HTML components. Then in the facet we use separate lists for the fields
> and HTML components. The fields will have a reference to the
> corresponding HTML components. There can be more HTML components than
> fields. But this will require a significant restructuring.

Maybe we can use hybrid solution: html widgets would be simple object 
with some properties and create() method. They should not be called 
widgets. They would not be part of sections fields. Rendering would be 
done in widgets/sections create() method (as it is now).
Pros:
  - reusable layouts, headers...
  - not polluting field names
Cons:
  - not so declarative - need to override update method, create custom 
section/widget factories - same as now

Question:
  - what with widget nesting? rule_details_section is in fact a 
composite widget. I would like to keep the concept, because it offers 
better code reuse.

These questions should be considered when designing UI extending.

>>> 5. In details facet update(), instead of storing the callbacks in
>>> update_custom_on_win and update_custom_on_fail and requiring the
>>> subclasses to call them, it might be better to call them from the
>>> update() itself:
>>>
>>> var command = IPA.command({
>>> ...
>>> on_success: function(data, text_status, xhr) {
>>> that.on_update_success(data, text_status, xhr)
>>> if (on_win) on_win.call(this, data, text_status, xhr);
>>> },
>>> on_error: function(xhr, text_status, error_thrown) {
>>> that.on_update_error(xhr, text_status, error_thrown);
>>> if (on_fail) on_fail.call(
>>> this, xhr, text_status, error_thrown);
>>> }
>>> });
>>
>> There are two types of success/error handlers:
>> 1) the default ones in details facet
>> 2) the ones supplied to update(win, fail) method.
>>
>> In my implementation I have separated 1) from update method so if
>> default behaviour isn't suitable these methods can be overridden without
>> overriding whole update method. Overriding isn't required. This is IMHO
>> good (less code duplication).
>
> There is still code duplication, the subclass that overrides the
> on_update_success/error (#1) will still have to call
> update_custom_on_win/error (#2) manually. In the changes that I proposed
> you don't have to do that because #2 are called inside update().
>
Changed to proposed solution.

>>> 6. Can IPA.batch_details_facet be combined into IPA.details_facet? I
>>> think the base details facet should support both regular mod and batch.
>>
>> The only point where they overlap is the update method. It's possible to
>> add a logic to switch between command creations - maybe based on
>> attribute (could be defined in spec). But should we do that? Isn't this
>> the reason for inheriting the class?
>
> Here's a scenario: suppose there's a subclass of IPA.details_facet that
> are so useful that it's used in many places. Then suppose we want to
> create a subclass of it but we want to do batch operation too, we'd have
> to copy the code from IPA.batch_details_facet.
Will merge.

> I think the decision to use mod or batch should be separate from details
> facet. I'm thinking to create a separate class IPA.client which we can
> add commands into it and execute them either individually or as a batch.

What would be the IPA.client's responsibilities? It'll be only a 
container for commands with different executing strategies?

-- 
Petr Vobornik