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

Endi Sukma Dewata edewata at redhat.com
Mon May 7 23:47:32 UTC 2012


The code works, it's ACKed. I have some comments below:

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?

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

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.

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?

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 {
       ...
   }

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.

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.

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.

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.

-- 
Endi S. Dewata




More information about the Freeipa-devel mailing list