[Freeipa-devel] [WIP] Web UI Refactoring & plugins effort - current state

Thu Mar 28 11:50:29 UTC 2013

Thanks for the review, I will incorporate most of the agreed changes ASAP.

See comments below.

On 03/26/2013 03:08 PM, Endi Sukma Dewata wrote:
> Hi, I have some comments & questions.
>
> Navigation:
>
> * Dojo/topic is used to handle low-level UI events such as facet-show
> and facet-change. Is it possible to use dojo/on or direct method calls?
> I suppose topics would also run slower (although that might not be an
> issue).

The low-level nature might be debatable. I don't have a strong option 
though so I will change it.

Regarding the performance part:  Looked at source code and topic is 
basically a interface for global `Evented` object (and Evented 
internally uses dojo/on). So the performance is about the same.

> Dojo/topic seem to be more appropriate for high-level events such as
> phase changes (e.g. UI initialized), data changes (e.g. user added),
> auth changes (e.g. login/logout/expiration), etc. which might affect the
> UI globally. This way 3rd-party code can subscribe to these topics too.

+1

> * The navigation uses a 'hash' array that will be joined with a '/'.
> Should we call this a 'path'?

Sure. So joining path will produce hash, right? (I consider 'hash' 
everything in url after the hash sign '#'.

>
> * Is it necessary to have /e and /p prefixes? Is there a possibility
> where 'entity' and 'page' might conflict?
>

The general idea is to have means to use entity-less facets. It might be 
an error page, some help, quick actions or whatever (my favorite Web CLI 
idea :)).

The infrastructure is not yet fully implemented/tested. I will uncomment 
the code and try if its working.

Prefixes are there to distinguish following paths:
'/e/:entity'
'/p/:page'

It clearly says what route handler should be used. This pattern also 
seems more extensible.

In theory the prefixes can be removed and routes can be handled by 
single handler. I'm not sure if it would be a win though.

> * When is /p used? Any example?

Nowhere yet. First will be the error facet, which reminds me that I 
should implement proper error reporting for initialization errors (phase 
errors).

>
> * The paths probably can be made more REST-like. Suppose in the future
> IPA uses REST, they will be easier to translate. Look for REST URL best
> practices. This is the path style used in Dogtag:
>
>    /users                       -> Users search facet
>    /users/admin                 -> Admin's details (or initial) facet
>    /users/admin/memberof_groups -> Admin's member of groups
>

Nice idea, but not sure if it will add value. More REST-like approach 
suffers in cases when an entity doesn't have an implicit facet/view.

Two search and details facets in automember:
/e/automember/searchgroup
/e/automember/searchhostgroup

If we use the propose form add the facet after / it might be confused 
with pkey.

(Automember plugin is not designed well...)

I'm sure we can fine more special cases.

In theory we can make custom routes for different entities - make 
virtual entities. One problem in current router design is that its 
create_hash method is not exactly extensible in comparison with 
register_route - plugin/module can register new route handler for some 
paths but it can't tell the router to make hash in different ways.

Now, path structure basically copies UI structure, which is IMO beneficial.

> * When you hover over a link, the hash shown doesn't match the actual
> hash after clicking the link.

Seem like too much work for not much gain. On each update we would have 
to create hashes for each link, that means to know beforehand what the 
handler would do with facet state. Some links might not even change the 
facet state/hash.

The only reason why is there something is that 'a' element requires 
something in 'href' attr.

>
> * Changing page number doesn't refresh the content. In Permissions
> search facet if you click Prev or Next or entering the page number
> nothing is changed, but if you go to another facet then come back it
> will show the new page.

Fixed. There was a typo in Facet_state class.

I also noticed related problem. Strict value comparison '===' is used in 
facet.state_diff method. In general I consider it better than '==' 
because it helps to avoid surprises. It creates a problem for Numbers 
stored in a state.

For example: that.state.set({page: 1});

Serialization and then deserialization from hash changes 1 to '1'. Which 
makes state_diff to return true.

Question is if it's better to say that all simple values stored in state 
should be strings or whether it is better to use == for comparison in 
state_diff.

>
> * Existing issue, the navigation profile is hard-coded so it's either
> admin or self-service, and the identity/user navigation item is defined
> twice. Is it possible to generalize this so the navigation items are
> defined only once and each item (or the controller) will determine
> whether it's supposed to be visible? In the future it might be possible
> to show more read-only navigation items in self-service mode, so it's
> not only limited to identity/user.

I didn't give much thought into profiles yet. I basically transformed 
existing functionality.

I agree that it should/might be improved. Not sure what is the best way. 
For example I was thinking about, that menu would not be defined 
centrally like now but each module would add its own menu items in 
profile phase. It might bring some problems like ordering of items, to 
many refreshes and so on, but that's manageable.

As an example I can mention reversed used case - removal of DNS menu 
item when dns is not configured.

   phases.on('profile', function() {
       if (!IPA.dns_enabled) {
           menu.remove_item('identity/dns');
       }
   }, 20);

>
>
> Registers:
>
> * The 'register' objects probably should be called 'registry'.

OK. I wasn't sure what's better.
>
>
> Phases:
>
> * What's the advantage of using dojo/topic to define phases vs. using
> inheritance to override phase methods?

dojo/topic only notifies about phase changes and it's main purpose is to 
give some controller information about what is happening. At the moment 
it's only used for handling 'phase-error' event.

I tried to avoid using inheritance/method overriding for the purpose of 
altering of initialization process. Therefore I created phases as they 
are. Phase modules are not dependent on any other modules so each module 
can register it's task independently on other modules. It means they 
tasks before/after other tasks (each task can have priority set).

Probably main benefit is that phases uses Deferreds. So we can 
synchronize asynchronous tasks - phase is not finished until all tasks 
are completed.

>
> * Any example where a phase has multiple tasks?

Extensibility. Some module might require to run a command to check if 
some feature is supported. Something like following:

(using dns as if it was an external plugin)

   var dns_module = {
       enabled: false,
       check_support: function() {
           var deferred = new Deferred();
           var command = IPA.command({
               entity: 'dns',
               method: 'is_enabled',
               on_success: lang.hitch(this, function(data, text_status, 
xhr) {
                   this.enabled = data.result;
                   deferred.resolve(this.enabled);
               }),
               on_error: function(xhr, text_status, error_thrown) {
                   deferred.reject(error_thrown);
               }
           }
           command.execute();
           return deferred.promise;
       }
   }

   phases.on('init', function() {
       return dns_module.check_support();
   });

   return dns_module;

We can see that there is no need to hack IPA.init method.

>
> * The 'alternation' phase probably should be called 'customization'.

Sure

When I was writing plugin example I discovered, that it would be useful 
to be able to add some new phase after/before some existing phase to be 
able to do some work after all tasks are finished but before we proceed 
to next phase. Theoretically it can be done in a handler for 
'phase-completed' topic but it's limited only to synchronous tasks.

>
>
> Plugins:
>
> * Right now the Web UI plugin registration requires running a script to
> update the index file so the plugin can be loaded. Is it possible to
> write a server plugin that reads the content of the UI plugins folder on
> the server, and have the UI call it to get the list of UI plugins and
> then load it dynamically?

We can make a wsgi plugin to run a simple script. I don't like that it 
would change the nature of static UI + it's necessary to run it every 
time we load an UI. On the other hand it simplifies installation of 
FreeIPA and its plugins.

I don't have a strong option on either solution.

>
> * Is there a base class for a plugin?

No

> Or is a plugin basically a
> collection of custom UI components (e.g. entities, facets, widgets)?

Yes

>
> * Any plugin example? How do you customize the menu (e.g. add a new page)?

Some examples attached - plain source code and also a patch that can 
applied and examined in static version.

Example shows, that adding new entity is really easy but adding another 
entity subtree is not really straightforward.
>
>
> Other:
>
> * The HTML elements in widgets/App.js still contain 'id' attributes
> which is a global identifier. Not really a problem since it's only used
> once in UI, but to be really modular it should not contain global
> identifiers.

Yes, we should remove them.

>
> * This is probably an existing issue, in self-service mode if you select
> one of the memberof tabs you'll see the Add & Delete appear briefly then
> disappear immediately. Ideally they should appear only if the user has
> add/delete rights.

Separate issue, will be fixed later.

>
> * Also in self-service mode, the user can see the action drop down list
> and action box (of other users) but none of them actually works.

Separate issue, will be fixed later.

By testing some stuff for this mail I noticed several errors in DNS and 
automember plugin, will fix them.

-- 
Petr Vobornik
-------------- next part --------------
A non-text attachment was scrubbed...
Name: simpleuser.js
Type: application/javascript
Size: 6564 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130328/32bfe701/attachment.js>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: wip-freeipa-pvoborni-0463-Another-user-pages-plugin-example.patch
Type: text/x-patch
Size: 14694 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130328/32bfe701/attachment.bin>