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:                      +-----------------+

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

Freeipa-devel mailing list

Reply via email to