EasySOA-Registry-Core code cleanup

tiry commented 12 years ago

I did a very quick review of the code and tested the app inside Nuxeo 5.4.2.

Here is some feedback on things that could be improved.

Seam beans

I would clearly move the seam beans in a separated module.

The registry-web module seems to be a nice candiate for hosting them.

This is not strictly required (it does work with current layout), but this makes sense in terms of layer separation and this would allow to use Nuxeo IDE with full Hot Reload of the Seam part.

ContentTemplate

Nuxeo Platform has an extension point to define what the initial Content Layout must look like.

From my understanding what you have in DomainInit / EasySOAInitComponent could easily move to a content template.

See Content Template Extension Point

As for the default users, you can contribute them via a CSV file => we can see this together if needed

NB : In addition of avoiding uneeded infrastructure code, it would also allow to leverage existing code that already handle TX issues at startup that may be diffrerent depending on the target Database : so this would make the current code shorter and more resilient

Sync Listeners

There are several sync listeners.

One of them uses HttpFile to fetch a WSDL and parse it.

This is very dangerous to do it synchronously and this won't work if the server if behing a proxy.

I would say that it is probably better to move it in async and use an Http pool that use Nuxeo http proxy config, but this is an improvement and not an urgent matter.

NB : for that I may have some small work on the Nuxeo side to make available the http pool as a service, but once we are aligned on 5.6 this should be a matter of hours.

JSF WebPart

EasySOA contains overides of the default templates.

Even if we technically allow that, this not the best option, at least in terms of upgrade.

We should be able to contribute all specific display using actions and widgets and remove all overrides.

For the EasySOA submenu, may be it would make sense to make it a real tab and use the AdminCenter/Home structure so that you can add all the required EasySOA specific config screens.

=> to be discussed (and done) during a workshop between Marwan and me

LifeCycle

I see several fields in your XSDs that really looks like lifecycle state.

I don't see why we can not use the native LifeCycle system from Nuxeo.

If needed you can see example here and a short introduction here.

API names

Main API for changing your services is notifyXXX.

For me this sounds like an event driven (async) API, but it's not.

In addition the Map<String,String> is good for an event driven API, but not so good if you want a real API (no type safety and loose coupeling).

=> am I missing something ?

mdutoo commented 12 years ago

I'm OK with most of it, very sound advice from a Nuxeo guru.

Especially lifecycles, I'm sure you can bring improvement there.

EasySOA submenu is cool but requires functional design first.

TODO mkalam Nuxeo Admin Center's proxy conf should also be used by others UIs, through a specific UI init WebEngine service.

mkalam-alami commented 12 years ago

I agree with these suggestions too.

LifeCycle: Actually we have several concepts that could fit the Nuxeo lifecycle system, mainly:
- An approbation lifecycle (that's what is currently - partially - implemented), in order to inform about the information quality.
- A service lifecycle (from design to deployment), currently stored in soacommon:lifeCycleStatus
API names: Indeed the names can be misleading, we're just using "notify" in the sense of "inform that something has been discovered". Now the use of the Map felt appropriate because what we're doing is actually close to the "Update Properties" service from the Content Automation, only with a bit of specific behavior according to the document type. But indeed this is atm a low-level API with poor business interface.

tiry commented 12 years ago

Before doing any upgrade, we must remove as much as possible all template override :

avoid possible compatibility issue at upgrade time
avoid loosing core features by overriding template with old version

Here a more detailed list of points that should be addressed.

Themes

Failed Themes widgets overrides

easysoa_breadcrumb_vn.xhtml
easysoa_tree_explorer_with_virtual_nav.xhtml

These 2 templates have few changes and anyway do not override the default one because the override key take the caching parameter into account.

This may be seen as a theme engine bug, but as it's visible in the Theme Editor the next EasySOA fragements are here but not bound to the theme.

Anyway the caching parameter is probably not what you want to change

=> either remove the override

or

=> remove the no-cache

CSS

The EsaySOA CSS is contributed to the theme, it's ok.

But starting with 5.5, we should be able to do this by simply using CSS and Flavors : no need to change the Themes resources.

View Overrides

document_comments.xhtml

The override is here to add custom display depending on group.

So far this overide is not really dangerous.

=> could be done better by adding a customizable layout in Nuxeo for the comments

==> nothing for now, add layouts in 5.6

document_view.xhtml / easysoa_view.xhtml

The default view is a layout that displays summary widgets.

=> to add easysoa_view.xhtml, you just have to wrap it in a custom widget and update the layout definition.

<extension target="org.nuxeo.ecm.platform.forms.layout.WebLayoutManager"
   point="widgettypes">

 <widgetType name="easySOAView">
  <configuration>
    <description>
        This widgets displays ...
    </description>
    <categories>
      <category>summary</category>
    </categories>
    <supportedModes>
      <mode>view</mode>
    </supportedModes>
  </configuration>
  <handler-class>
    org.nuxeo.ecm.platform.forms.layout.facelets.plugins.TemplateWidgetTypeHandler
  </handler-class>
  <property name="template">
    /widgets/summary/easysoa_view.xhtml
  </property>
 </widgetType>

</extension>

content_view.xhtml

It would be better to override document_content.xhtml or even better to define an easysoa_document_content_view.xhtml

=> would avoid impacting all views using content_view.xhtml

=> would avoid loosing changes during updates

content_view_search_layout.xhtml

Why removing this : it seems too bad to have an override to remove features :) If really needed you can just override the contentview definition and configure it to not show the filter.

=> you can define the search layout, but also change it's display type

=> you can but it to none if needed ...

=> see contentviews-contrib.xml

  <searchLayout name="document_content_filter"
    filterDisplayType="quick" />
  <showFilterForm>true</showFilterForm>

content_view_result_layout_selector.xhtml

Looks like it is overiden to remove all page size / display types selectors.

It seems too bad to have an override to remove features :)

=> you can simply change the default content view config to hide the selectors and have only one result layout

 <resultLayouts>
    <layout name="document_listing_ajax" title="document_listing"
      translateTitle="true" iconPath="/icons/document_listing_icon.png"
      showCSVExport="true" showPDFExport="false" showSyndicationLinks="true" />
    <layout name="document_listing_ajax_compact_2_columns" title="document_listing_compact_2_columns"
      translateTitle="true" iconPath="/icons/document_listing_compact_2_columns_icon.png"
      showCSVExport="true" showPDFExport="false" showSyndicationLinks="true" />
    <layout name="document_listing_ajax_icon_2_columns" title="document_listing_icon_2_columns"
      translateTitle="true" iconPath="/icons/document_listing_icon_2_columns_icon.png"
      showCSVExport="true" showPDFExport="false" showSyndicationLinks="true" />
 </resultLayouts>

Look and Feel

nuxeo_header.xhtml

This override is here mainly for adding the EasySOA Menu.

You should not overide the complete template : here it looks like the complete nuxeo_header_template.xhtml is duplicated.

In addition, all menus entries are managed by actions :

you can de-activate the entries you don't want
you can add the one you want

=> this would be better that brutal override

An option is also to directly add a new EasySOA main tab and create a dedciated Admin Center like view

=> this is only really meaningful if you have more than one entry to add :)

logo stuffs

Ideally we should remove the template overrides : overding the picture would be better and enough. When 5.5 alignment is done, all this can not be more easily contributed via Theme Flavors.

==> I'll do the changes this week

mdutoo commented 12 years ago

When template overrides are removed, remove the warning preventing Nuxeo Studio at https://github.com/easysoa/EasySOA/wiki/Configure-using-nuxeo-studio

tiry commented 12 years ago

be patient : work is scheduled for Thuesday :)

mkalam-alami commented 12 years ago

Here is a technical question: as we discussed during the workshop, I'm experiencing more infinite loops problems with a new feature, so I'd like to use the DocumentModels' "contextData". However I couldn't use it as I want, since the data is not kept during saving:

        // In some component, I run this:
        service.putContextData(Service.CONTEXTDATA_NOVALIDATION, true);
        session.saveDocument(service); 

        [...]

        // In any listener triggered while saving, this condition is never satisfied
        if (doc.getContextData(Service.CONTEXTDATA_NOVALIDATION) != null) { 
             [...]
        }

I guess the question is: in which scope is the ContextData kept exactly ? Thanks

tiry commented 12 years ago

The contextData anyway does not last more than one transaction.

Tiry Le 31 janv. 2012 17:55, "Marwane Kalam-Alami" < reply@reply.github.com> a crit :

Here is a technical question: as we discussed during the workshop, I'm experiencing more infinite loops problems with a new feature, so I'd like to use the DocumentModels' "contextData". However I couldn't use it as I want, since the data is not kept during saving:
       // In some component, I run this:
       service.putContextData(Service.CONTEXTDATA_NOVALIDATION, true);
       session.saveDocument(service);

       [...]

       // In any listener triggered while saving, this condition is never
satisfied
       if (doc.getContextData(Service.CONTEXTDATA_NOVALIDATION) != null) {
            [...]
       }
I guess the question is: in which scope is the ContextData kept exactly ? Thanks

Reply to this email directly or view it on GitHub: https://github.com/easysoa/EasySOA/issues/82#issuecomment-3742919

mkalam-alami commented 12 years ago

Ok, thanks. I thought since about using custom event types instead (ex: eventProducer.fireEvent(validateDocumentEvent) when I want to validate a service), it doesn't directly solve everything but it actually helps keeping things simple and clean.

tiry commented 12 years ago

Indeed, using "application level" event allows to have a better control over your listeners.

In addition if you configure Audit Service to log it, it will give you a better tracability.

easysoa / EasySOA