[Freeipa-devel] [PATCHES] 124-132 Inconsistent ways to show/change entry status

[Petr Vobornik]

On 05/08/2012 01:47 AM, Endi Sukma Dewata wrote:
> The code works, it's ACKed.

Patches 124-132 pushed to master.

> I have some comments below:

I'll address some points in separate patches.

>
> On 5/2/2012 8:33 AM, Petr Vobornik wrote:
>> This bunch of patches are implementing ticket #2247. They introduce some
>> new logic and types of internal objects. There might be design issues
>> (mainly in state evaluation). I would appreciate some opinions on what
>> might be improved.
>>
>> See patch comments for more details.
>>
>> What I think might be the main concerns:
>>
>> Action list definition:
>> Now action lists are defined on facet level and facet header takes them
>> from facet. Would in be better to define action list - the widget and
>> actions separately. The widget could be defined in header and actions on
>> facet level.
>
> Yes, the header could be considered a widget, so it contain the action
> list widget.
>
>> State evaluation:
>> The patches are adding support for some kind of state evaluation. The
>> state is represented by array of string. I'm thinking that the design
>> might not be robust. Is a string good enough? We might have a problem
>> with conditions like to "have this particular access right' (#2318).
>> There are state_evaluators and state_listeners. They are practically the
>> same but the execution point and parameters are different. Should they
>> be somehow joined?
>
> It might be better to use a map to represent the state. The map could
> hold any attribute types, not just string flag with the current array.
> So, the widget itself could be the state because it's also a map. That
> said, evaluating a map might be difficult to define declaratively, we
> would end up using a code again. So for now using an array of string is
> fine, but we just have to know the limitations.
>
>> Code placement:
>> There's a lot of new objects and for some of them it is not clear to
>> what code file they should be placed.
>
> This will continue to change as we add more code. Feel free to create a
> new file when you see a pattern. We might want to start separating the
> IPA-specific code from the framework (reusable code). The framework
> could be moved into a 'lib' folder.
>
>> FYI: In close future I would like to address some problems in UI
>> architecture. I'm in a middle of designing phase, so there is nothing to
>> present at the moment. The main topics are:
>> * reduce the need of overriding methods when a new widgets or
>> capabilities are added
>> * make it more declarative to enhance extendibility
>> It may be done by:
>> * better inheriting model to support events
>> * build phases (preinit, init, postinit, create, load) to improve spec
>> object creating and initialization of created object.
>> * path representation of an event/attribute/model property to support
>> bindings to various events/attrs from anywhere
>> * introduce model and model bindings, converters between command
>> output/model/human readable representation
>
> Sounds good! Some comments about the patch:
>
> 1. When you open a specific user, there's a default action that's
> already selected in the action list. The thing is the current default
> action (i.e. Disable) is probably not something that's regularly used.
> It might be better to show something like '-- Select Action --' or '--
> Action List --' so you'd have to select an action first then click Apply.
>
> 2. Suppose we did #1, do we still need the confirmation when applying
> the action?

We don't.
>
> 3. I think the action list should be available in all details page for
> all entities. At least it should have the Delete action.

#1,#2,#3: Sounds reasonable, will do.

Delete button can be also created as control button and we have ticket 
for it. I'll do it as action in action list, though.

>
> 4. Something to think about, do we need an action list in the
> association page? Entities like group default to an association page
> instead of details page. So if we don't have an action list in the
> association page the UI may look inconsistent because you'd have to go
> to the Settings to execute an action. But if we do have an action list
> in the association page, the actions could become confusing: do they
> apply to the associations or to the entry itself? One possible solution
> is to move the Settings to the left-most position and make it the
> default page and have the action list in the details page only.

I don't think there is an optimal solution. For now I prefer current 
implementation. I agree with your thoughts.

Reasons why would I leave it as is:
   * if association facet is the first facet it usually means that the 
details facet is less important and so the actions are not so important 
(frequently used) to be offered in assoc. facet. User can always switch 
to settings facet. Nice example is dnszone entity or other nested facets.
   * I would show action list in association facets only for actions 
related to associations even though the association is related to the 
object.

>
> 5. Some details pages have status indicator (check sign or minus sign)
> on the left of the page title, some others don't. This is because not
> all entities have a concept of status. Would it be better to show the
> status as a read-only field in the facet content?

I just want to show read only field as an addition to the icon or to 
replace the icon?

I have nothing against showing it as an addition.

>
> 6. It might be better to avoid using element ID in the CSS. This would
> make the CSS more reusable.
>
> #content .facet-action-list div[name=apply] a.ui-state-default {
> ...
> }

I'll try to avoid it. I'm also thinking about using OOCSS principals. 
They seems very useful for the kind of application which IPA Web UI is.

>
> 7. In some facet class definitions the no_init parameter is defined
> separately from the spec object, any particular reason?
>
> IPA.facet = function(spec, no_init) {
> ...
> };
>
> You can think of spec object as named parameters. So the no_init can be
> defined in the spec object and later used to determine what operations
> to be done inside the init method.

It is to suppress initialization of ancestor classed before the process 
of inheriting is completed.

If I would alter the spec it one class I would have to remember the 
state before calling ancestor constructor because offspring classes 
wouldn't be able to suppress this class init method.

Example where current class wants to disable init method in ancestors 
builder function:

var no_init = spec.no_init; //remember offspring will
spec.no_init = true; //disable init in ancestor - IPA.facet
var that = IPA.facet(spec);
if (!no_init) that.init(); //my init

Current implementation seemed easier.

I already have a concept which can replace this.

http://pvoborni.fedorapeople.org/patches/wip-freeipa-pvoborni-0123-IFW-base-classes-builders.patch
http://pvoborni.fedorapeople.org/patches/wip-freeipa-pvoborni-0124-Events.patch


>
> 8. The declarative control button definitions still contain some code,
> e.g. the handler function inside the button actions. Maybe the actions
> should be defined in a separate list and the buttons can refer to them
> by name.

This separation sounds interesting. We will be able to define some list 
of actions which applies to that object and then action_list, 
control_buttons and action_panels can pick whatever they want.

But the code you are referring to is just a shortcut to safe some lines 
of code - lets say anonymous action.

>
> 9. The 'control_buttons' attribute in search facet definitions contains
> a 'buttons' array. Any plans to create custom control buttons widget? If
> not maybe the 'control_buttons' itself could be the array.

No plans but in 'control_buttons' definition can be also added 
state_listeners.

>
> 10. The 'Action List' in ticket #2248 for the password reset is actually
> different than the action list for Enable/Disable. I was proposing to
> create a small panel under the 'Account Settings' section, probably
> something like this:
>
> ---------------------------------------------------------------
> Account Settings
> +-----------------+
> User login: admin | Actions: |
> Password: | Reset password |
> Password expiration: +-----------------+
> UID:
> GID:
>
> This panel might better be called 'Action Panel' to distinguish from the
> dropdown 'Action List' on the top. The reason for this panel is that a
> Password Reset only affects an aspect of the user, not the entire object
> like Enable/Disable (although that can also be argued differently), so
> the action should be placed where it's relevant, in this case near the
> Password field. The same concept will be used for ticket #2250, #2251,
> #2252.
>

I'll consider my patch #133 NACKed and first create the action panels.

-- 
Petr Vobornik