[Freeipa-devel] Web UI refactoring effort ready for review

[Petr Vobornik]

On 05/03/2013 07:35 PM, Endi Sukma Dewata wrote:
> Hi,
>
> Sorry for the delay, I have some questions & comments.
>
> Registry:
>
> In the simpleuser.js the new 'user' entity is registered first then the
> old 'user' entity is removed, which could be confusing because they are
> both identified using 'user'. Should register() automatically remove the
> old object?

I've mixed feelings about it. I would rather keep the methods simple - 
one task oriented.

The issue was caused by other problem: The plugin had to wait for 
metadata and profile information (to check self-service). So the 
callback is called after menu is created and menu requires the entity to 
be resolved. Should we postpone the menu creation to a different phase? 
Maybe a new one?

> Ideally a class should have complete methods to manage the
> objects it stores (e.g. unregister(), remove()).

This can be added.

>
> How is reg.entity created? Are there others beside 'entity'?

Registries are created in related modules. So reg.entity get 
instantiated at the end of entity.js.

There is one for each object type, which are, IIRC: action, facet, 
entity, field, validator and some others.

>
> How is Registries_registry in reg.js used? It doesn't seem to be used
> anywhere else.

Wow, I forgot about this peace of code. It isn't and should be removed.
>
> Plugins:
>
> In plugins.py the list of plugins is generated using os.listdir(). Then
> each plugin also has a list of dependencies which I suppose can include
> other plugins. Then when registering the plugin task, it will have a
> priority as well.
>
> So there seem to be several factors that determine the execution order
> of the plugins. There should be a document explaining how this will
> work, so plugin writers can be sure that the code will be executed at
> the right time.
>
> In general I'd avoid using task priority because it doesn't guarantee
> the correct execution order unless the priorities of all tasks are well
> coordinated (which might be challenging if there are multiple plugins
> owned by different people).

You are right and it gets more complicated with asynchronous tasks. If 
one wants to react to an asynchronous task, he has to do it next phase.

We might set some fixed priorities for certain operations.

Anyway the documentation is essential.

>
> Could you add more examples of simple plugins for various scenarios
> including custom entity, custom facet, custom field, custom menu? They
> can be included in the RPM for reference.

Yes, I that's my plan. Another reason to do it to find limitations.

>
> Writing a plugin seems to still require programming skills, reliance on
> good docs, and probably even some source code familiarity. What do you
> think about simplifying this a little further? So we'll have 2 ways to
> define a plugin: one is programatically using the current framework
> already implemented (e.g. simpleuser.js), and the other is completely
> declaratively using a plain json data (e.g. simpleuser.json). The
> declarative plugin will obviously be more limited, but much simpler to use.

I agree with the idea. But before creating declarative JSON format we 
should come up with a plugin API, which would be more-or-less stable and 
therefore plugins might be resilient to Web UI internals changes.

When we have this API we might map it to a JSON representation.

The hard part will be to find all the use-cases to cover.

>
> Builder:
>
>> b) Second big issue was build of objects. Entities and facets have
>> complex build logic. It can be simplified into three steps:
>>      1) modifications of spec
>>      2) creation of object and class inheritance
>>      3) init logic
>
> Yes, creating an object has become very complicated now with the
> builders, factories, constructors, preops, postops, inits, overrides,
> diff, etc. I think the problem is that we're trying to create/modify the
> spec before creating the object and we need a whole set of mechanisms to
> do that. Maybe we can simplify it into two basic steps:
>
> 1. Create an empty/simple object.
> 2. Initialize the object.
>
> The initialization process could be split further into smaller
> operations such as:
>
> * Load the spec and modify it if necessary
> * Creating dependent objects and initializing them
> * Other initialization steps
>
> The builder, factory, preops, and postops can be included as part of the
> initialization step. They can be normal class methods rather than
> loosely defined functions and can be overridden by subclasses. There's
> probably a lot more details that need to be discussed.

+1 Rewriting all the factories into classes will be a huge task though.
At the moment, the biggest problem are spec modification which are not 
defaults (so they can't be overriden). Like the ones described bellow - #2.

>
>> 1. Move ./_base/metadata_provider to ./metadata?
>> Might simplify stuff.
>
> This seems to be IPA-specific, so yes.

https://fedorahosted.org/freeipa/ticket/3604

>
>> 2. Move actions/buttons spec from factories to pre_ops associated with
>> the factories.
>>
>> Example of stuff to be moved (search.js):
>>       spec.actions = spec.actions || [];
>>       spec.actions.unshift(
>>           'refresh',
>>           'batch_remove',
>>           'add');
>>
>> It may simplify/allow removal of those items in spec or pre_ops of child
>> factories. Currently there is no way how to intercept them.

https://fedorahosted.org/freeipa/ticket/3605

>
> Sure, I don't see any problem with that.
>
> In general there is no major issue that would warrant a NACK. As long as
> the API is well documented for plugin writers it should be sufficient.
>

-- 
Petr Vobornik