[Freeipa-devel] [PATCH] 647-651 [webui] Make utility section of navigation extensible

[Petr Vobornik]

On 20.6.2014 18:18, Endi Sukma Dewata wrote:
> On 6/18/2014 6:11 AM, Petr Vobornik wrote:
>>> 1. As discussed on IRC, the plugin is causing an error due to missing
>>> extend.js. This needs to be fixed.
>>
>> Fixed
>
>>> 4. I agree that the facet shouldn't define the hash. The hash should be
>>> part of the plugin declaration.
>>
>> Ideally, facet should be router agnostic. We need a mechanism of mapping
>> facet's state to hash and vice versa.
>>
>> Originally I did not want to do it now because it requires bigger
>> refactoring of a router.
>>
>> While I was designing it, I ended up with a patch - attached.
>>
>> It's a replacement for original patch #649. Patch #651 and example
>> plugin are updated accordingly.
>>
>> Basically I refactored router's `navigate_to_*`, `_create_*_hash` and
>> `*_route_handler` methods into separate classes which are registered and
>> mapped to facets. All in navigation.routing module.
>>
>> Btw, can you think of some better name(s)/placement for the module?
>
> ACK. Some comments below, but I think they can be addressed later.

pushed to master:
* c6c7dfeefbb8b94f102aef6a04d828530e3ccb0f webui: fix excessive 
registration of state change event listeners
* 27836cba9d865b1c912a65d0cd04562194f9e93f webui: support standalone 
facets in navigation module
* 86898065b5e1d60168e2daff050853729b34f1ce webui: generic routing
* 6f5e80b0cec57a89a68f2935b5fe01d919b11031 webui: add parent link to 
widgets in ContainerMixin
* 6e43d01266f105cdf3cf27a1dbb87ac80da4a06d webui: plugin API

>
> 1. I'm not sure if we really need a HashCreator. Ideally the router
> should map a hash to a page. Links to another page can be hardcoded too
> (and substitute the parameters).

The main purpose of a hash creator is to update hash when a facet state 
changes. This change can be initiated from the facet itself, e.g. when 
searching in a search facet. Removing the hash creator would make facets 
dependent on navigation specifics.

I would agree if it was related only to links. But even then it would be 
better to have a method which would create the hash (for the one which 
shares the same pattern) to reduce code reuse. In any case, it's 
possible to hardcode hash parts in links if one needs to.

>
> 2. Ideally there should be no entity-specific navigation code. All
> routing should be handled in a generic way. This probably depends on the
> entity-facet separation that we discussed previously. So routes like this:
>
>    /e/:entity/:facet/:pkeys/*args
>
> should be replaced by individual routes for each entity:
>
>    /user/:facet/:pkeys/*args
>    /group/:facet/:pkeys/*args
>

Yes, but IMHO the hash prefix is a detail. It would be more important if 
it would be a REST API where it's a core aspect.

> Probably the entities should be written like plugins.

They are, in a way... I'm toying with the idea of moving them (user.js, 
group.js, ...) to plugins dir.

>
> 3. If we fix #1 and #2, is the "routing" module still necessary? The
> "routing" object probably should have been a "router", but are you
> concerned about conflicting with Dojo's router and the existing Router
> module?

Our 'Router' is just an encapsulation over the Dojo's router. It's true 
that with this refactoring, the encapsulation became so thin that the 
logic could be moved to 'routing' and 'routing' could be then renamed to 
'router'. There would be no conflict since the modules are in different 
packages.

If we want to do it, we should do it in 4.0 (ASAP).

>
> 4. The args/query in the navigation paths should be separated by a more
> standard delimiter "?" instead of "//". For example:
>
>    /ipa/ui/#/e/user/search//filter=test
>
> should be replaced with:
>
>    /ipa/ui/#/e/user/search?filter=test
>

note that // is a result of /:pkeys/, where :pkeys is '' Therefore a 
simple replacement would lead to
     /ipa/ui/#/e/user/search/?filter=test

Please file a ticket, if you think it's important.

Lets say that we would implement #2 and #4. Would we also want to keep 
the old routes for backwards compatibility? (main purpose of hashes is 
to make the page bookmarkable). IMHO people don't care about #2 and #4 
much, but they care about broken bookmarks.
-- 
Petr Vobornik