Details

    • Type: Wish Wish
    • Status: Open
    • Priority: Minor Minor
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: framework
    • Labels:
      None

      Description

      Hi devs,
      while discussing in the ML about modules and framework separation I thought to this new feature that I would like to discuss here with you.

      We have now the possibility to extend a menu from one other. This is great in order to have an high level of code reuse and great consistency all over OFBiz.
      I was thinking to a sort of "merges-to" property for the menu widget.
      This would allow a new module to specify an already exixting menu name (in the framework core or in a lower level module) that should be somewhat changed by the actual menu.

      For instance, in the attached image partymenu.jpg there is a a tipical use of this feature:
      in the party module there are lot of links that co to order application, account etc. Those menu link could be used defining a simple menu (say it partylinks_menu) in the party application that contains only party or framework related links (i.e. profile); additional components like order or accounting could define more menus that merges-to the partylinks_manu so that when the menu is rendered IN THE PARTY APPLICATION the new menu items added in the order and accounting applications are also rendered.

      This would allow us to dramatically reduce the component dependence and help us to have the framework-only distribution.

      To eventually implement this I think there should be an entity that defines such mergin menus and the menu rendered should lookup the entity to check if one or more merges to the actually rendering menu is defined.

      I would appreciate to hear from you if this idea can help.

      1. injections.patch
        34 kB
        Scott Gray
      2. googlebase-inject.patch
        25 kB
        Scott Gray
      3. injections.patch
        32 kB
        Scott Gray
      4. links.jpg
        312 kB
        Bruno Busco
      5. partymenu.JPG
        27 kB
        Bruno Busco

        Activity

        Hide
        Michael Xu added a comment -

        hi Bruno,

        This is really a great idea, which is similar with extension points in OSGI. Basically, each module can define its own extension (simple menu in this case) and then system can automatically load and aggregate them from different extension points in different components.

        Just my thoughts here:

        • access control on menu item level
          (If one user have no appropriate permission(s), then he should not even see the menu item.)
        • make the UI more flexible
          In the attached screen, I would like to suggest to put a "actions" dropdown list to contain all menu items. Otherwise, the layout might be likely get messed up. Of course, different people could have different taste. So maybe it is better to have flexibility here.
        • Is it possible to link an icon for each menu item? still for UI purpose.

        +1

        Regards,
        Michael

        Show
        Michael Xu added a comment - hi Bruno, This is really a great idea, which is similar with extension points in OSGI. Basically, each module can define its own extension (simple menu in this case) and then system can automatically load and aggregate them from different extension points in different components. Just my thoughts here: access control on menu item level (If one user have no appropriate permission(s), then he should not even see the menu item.) make the UI more flexible In the attached screen, I would like to suggest to put a "actions" dropdown list to contain all menu items. Otherwise, the layout might be likely get messed up. Of course, different people could have different taste. So maybe it is better to have flexibility here. Is it possible to link an icon for each menu item? still for UI purpose. +1 Regards, Michael
        Hide
        Scott Gray added a comment -

        I'm in favor of being able to extend existing menus (in place) but I'm not sure about using an entities to track them. Perhaps we could add a new widget-resource tag to the ofbiz-component.xml files that reads the extensions at startup?

        Slightly off topic but another thing I've wanted to be able to do for a while is to extend controller.xml files in the same manner as this.

        Show
        Scott Gray added a comment - I'm in favor of being able to extend existing menus (in place) but I'm not sure about using an entities to track them. Perhaps we could add a new widget-resource tag to the ofbiz-component.xml files that reads the extensions at startup? Slightly off topic but another thing I've wanted to be able to do for a while is to extend controller.xml files in the same manner as this.
        Hide
        Bruno Busco added a comment -

        Thank you Michael and Scott for your comments.

        • access menu - of course, the menu should be merged means that all menu-item with all their properties (and condition) should be considered
        • "actions" dropdown - should this be manages as a new menu type?
        • icons - I don't now, I guess it should be possible not looked into it yet
        • widget-resource in place of entity - yes it makes great sense

        more?

        Show
        Bruno Busco added a comment - Thank you Michael and Scott for your comments. access menu - of course, the menu should be merged means that all menu-item with all their properties (and condition) should be considered "actions" dropdown - should this be manages as a new menu type? icons - I don't now, I guess it should be possible not looked into it yet widget-resource in place of entity - yes it makes great sense more?
        Hide
        Scott Gray added a comment -

        Attached patch adds support for injecting controller entries and menu items.

        It works well but the code is pretty rough. Hopefully someone can clean it up a bit, otherwise I'll get back to it when I have time.

        Show
        Scott Gray added a comment - Attached patch adds support for injecting controller entries and menu items. It works well but the code is pretty rough. Hopefully someone can clean it up a bit, otherwise I'll get back to it when I have time.
        Hide
        Bruno Busco added a comment -

        Thank you Scott!
        I will try the patch soon.
        I have seen it does not include an example of injections. Do you have one available I could start from?
        If not I will try to create one on the example application.

        Show
        Bruno Busco added a comment - Thank you Scott! I will try the patch soon. I have seen it does not include an example of injections. Do you have one available I could start from? If not I will try to create one on the example application.
        Hide
        Scott Gray added a comment -

        Sorry I should have thought to include some sort of example.

        This is the best I have, a conversion of the google base component from a separate webapp to a plugin for the category webapp. Note that it is only an example and not intended to be committed (in it's current form at least)

        The most relevant changes in the patch are to the ofbiz-component file and also the introduction of the GoogleBaseInjectionMenus.xml file

        Show
        Scott Gray added a comment - Sorry I should have thought to include some sort of example. This is the best I have, a conversion of the google base component from a separate webapp to a plugin for the category webapp. Note that it is only an example and not intended to be committed (in it's current form at least) The most relevant changes in the patch are to the ofbiz-component file and also the introduction of the GoogleBaseInjectionMenus.xml file
        Hide
        Bruno Busco added a comment -

        GREAT!

        Show
        Bruno Busco added a comment - GREAT!
        Hide
        Adrian Crum added a comment -

        It seems to me we are going in the wrong direction here. From my perspective, the party manager application should only contain screens for managing a party. I've always had a problem with those links to other applications, and things have grown worse recently with menus in the party manager for employment and such - menu items that may not be applicable for that party. In the current trunk there are 20 tabs on the view profile page, and most of those tabs shouldn't be there. To summarize, too much external functionality is being forced into the party manager application.

        From my perspective, it should be the other way around. The party manager application should have screens and services to manage a party - and nothing more. Other applications extend those screens and services for specific party roles. For instance, an order manager application would have a Find Customer or Find Supplier screen, and that screen would constrain the Find Party list to customers or suppliers. The application would ADD the Orders link to the list in the screen shot. In the Customer/Supplier view profile screen it would ADD the Tax Infos, Rates, Shopping Lists, Fin History, and Product Store Roles tabs to the menu.

        In a similar way, a human resources application would extend the party profile screen and add the Party Skills, Resumes, and Employment Applications tabs to it.

        Although the party manager is being used as an example here, I've seen the same abuses/misuses done in other applications.

        I don't mean to throw a wet blanket on this effort, I just think the end result of this issue will encourage even more bad designs.

        I haven't tried Scott's patch, but a cursory look at it gives me the impression that applications can change the widget models on-the-fly. I don't think that is a good idea. In the current implementation, a Freemarker template gets a list of all mounted applications and creates a main navigation menu based on the user's permissions. In other words, the list of all applications is filtered down to the applications the user is permitted to use. Maybe I'm missing something, but Scott's patch doesn't seem to duplicate that behavior. I'm assuming each application menu item that is "injected" will have some kind of permission checking tied to it.

        Anyway, these are things that need to be discussed.

        Show
        Adrian Crum added a comment - It seems to me we are going in the wrong direction here. From my perspective, the party manager application should only contain screens for managing a party. I've always had a problem with those links to other applications, and things have grown worse recently with menus in the party manager for employment and such - menu items that may not be applicable for that party. In the current trunk there are 20 tabs on the view profile page, and most of those tabs shouldn't be there. To summarize, too much external functionality is being forced into the party manager application. From my perspective, it should be the other way around. The party manager application should have screens and services to manage a party - and nothing more. Other applications extend those screens and services for specific party roles. For instance, an order manager application would have a Find Customer or Find Supplier screen, and that screen would constrain the Find Party list to customers or suppliers. The application would ADD the Orders link to the list in the screen shot. In the Customer/Supplier view profile screen it would ADD the Tax Infos, Rates, Shopping Lists, Fin History, and Product Store Roles tabs to the menu. In a similar way, a human resources application would extend the party profile screen and add the Party Skills, Resumes, and Employment Applications tabs to it. Although the party manager is being used as an example here, I've seen the same abuses/misuses done in other applications. I don't mean to throw a wet blanket on this effort, I just think the end result of this issue will encourage even more bad designs. I haven't tried Scott's patch, but a cursory look at it gives me the impression that applications can change the widget models on-the-fly. I don't think that is a good idea. In the current implementation, a Freemarker template gets a list of all mounted applications and creates a main navigation menu based on the user's permissions. In other words, the list of all applications is filtered down to the applications the user is permitted to use. Maybe I'm missing something, but Scott's patch doesn't seem to duplicate that behavior. I'm assuming each application menu item that is "injected" will have some kind of permission checking tied to it. Anyway, these are things that need to be discussed.
        Hide
        Scott Gray added a comment -

        We've had the ability to extend applications via new components for a long time. This doesn't take away from that.

        What it does provide is the ability to create new features for existing applications without needing to modify those applications directly. Personally I think this is sorely needed and clearly evidenced by some of the special purpose components that really don't fit well as their own webapp, e.g. googlebase (catalog), the ebays (catalog, ordermgr), oagis (ordermgr, catalog, facility).

        Say I've created a new set of reports for the order manager application, should I have a completely separate webapp just to deliver a few reports? Should I completely override the order manager in hot-deploy just to add a few apps? What if I want to distribute these reports for other people to use in their own applications, should I bundle up my customized hot-deploy order manager app and distribute that? What if the people I'm distributing to already have their own customized order manager app because they needed to add a couple of screens somewhere else, should they try and merge the two?

        With this approach you just drop in your plug-in component and boom, there the reports are in the existing application. The idea here is that you pick and choose from the available plug-ins in order to enhance the applications distributed with OFBiz to meet your needs.

        Show
        Scott Gray added a comment - We've had the ability to extend applications via new components for a long time. This doesn't take away from that. What it does provide is the ability to create new features for existing applications without needing to modify those applications directly. Personally I think this is sorely needed and clearly evidenced by some of the special purpose components that really don't fit well as their own webapp, e.g. googlebase (catalog), the ebays (catalog, ordermgr), oagis (ordermgr, catalog, facility). Say I've created a new set of reports for the order manager application, should I have a completely separate webapp just to deliver a few reports? Should I completely override the order manager in hot-deploy just to add a few apps? What if I want to distribute these reports for other people to use in their own applications, should I bundle up my customized hot-deploy order manager app and distribute that? What if the people I'm distributing to already have their own customized order manager app because they needed to add a couple of screens somewhere else, should they try and merge the two? With this approach you just drop in your plug-in component and boom, there the reports are in the existing application. The idea here is that you pick and choose from the available plug-ins in order to enhance the applications distributed with OFBiz to meet your needs.
        Hide
        Bruno Busco added a comment -

        Adrian,
        a pattern that I have seen to be very powerful is to have a unique reference page for each "topic" and have this improved as long as new features come available on the system.

        The new feature can become available installing new components but the user should not know if a new feature is contained in a new component or in the old component that has been changed.

        I mean, coming to the profile page example,
        the system user should only now that whatever is related to the party profile is in a certain page, say Party->Profile.

        Then if he is logged into a system where the only thing that is possible to do is to change the user name and password he will find only these UI elements.

        If, in the system, the party profile has more parameters becouse more business components are installed than the party profile page UI should grow up allowing all the available options and controls.

        Using the ofbiz standard extension mechanism forces the user to change the screen, or the application where to go as long as new components (with new screens) are installed on the system.

        Another example could be a unique "Settings" page where all settings for all installed components are added.

        When we introduced the Portal system we already did a step in this direction. Right now, if a PortalPage is available in a "base" component, a newly installed component can add, to that PortalPage, one or more Portlets defined by itself.

        This would allow a central define "Party Profile" or a "Settings" page to be added with "dinamically" elements Portlets and Menus.

        Show
        Bruno Busco added a comment - Adrian, a pattern that I have seen to be very powerful is to have a unique reference page for each "topic" and have this improved as long as new features come available on the system. The new feature can become available installing new components but the user should not know if a new feature is contained in a new component or in the old component that has been changed. I mean, coming to the profile page example, the system user should only now that whatever is related to the party profile is in a certain page, say Party->Profile. Then if he is logged into a system where the only thing that is possible to do is to change the user name and password he will find only these UI elements. If, in the system, the party profile has more parameters becouse more business components are installed than the party profile page UI should grow up allowing all the available options and controls. Using the ofbiz standard extension mechanism forces the user to change the screen, or the application where to go as long as new components (with new screens) are installed on the system. Another example could be a unique "Settings" page where all settings for all installed components are added. When we introduced the Portal system we already did a step in this direction. Right now, if a PortalPage is available in a "base" component, a newly installed component can add, to that PortalPage, one or more Portlets defined by itself. This would allow a central define "Party Profile" or a "Settings" page to be added with "dinamically" elements Portlets and Menus.
        Hide
        Adrian Crum added a comment - - edited

        Scott,

        Those are good requirements - thanks.

        Would a menu widget factory method solve that issue? Let's say your custom application registers a menu that overrides an existing menu and provides the additional items. The custom menu could extend (or wrap) the existing menu it replaces.

        Show
        Adrian Crum added a comment - - edited Scott, Those are good requirements - thanks. Would a menu widget factory method solve that issue? Let's say your custom application registers a menu that overrides an existing menu and provides the additional items. The custom menu could extend (or wrap) the existing menu it replaces.
        Hide
        Scott Gray added a comment -

        Adrian,

        I'm not entirely sure what you're suggesting, or more specifically, how it differs from what's in my patch. The patch does modify the menu model at run-time but that is on purpose, the idea being that these external add-ons transparently become an intrinsic part of the model being extended. In this way, even if you do have a hot-deploy order manager, the plug-in will still work so long as your custom application is extending the order manager's controller and reports menu. Unbeknownst to the hot-deploy app those order manager artifacts are actually composites of the base functionality plus any plugged in functionality. I actually think it's kinda cool.

        Show
        Scott Gray added a comment - Adrian, I'm not entirely sure what you're suggesting, or more specifically, how it differs from what's in my patch. The patch does modify the menu model at run-time but that is on purpose, the idea being that these external add-ons transparently become an intrinsic part of the model being extended. In this way, even if you do have a hot-deploy order manager, the plug-in will still work so long as your custom application is extending the order manager's controller and reports menu. Unbeknownst to the hot-deploy app those order manager artifacts are actually composites of the base functionality plus any plugged in functionality. I actually think it's kinda cool.
        Hide
        Adrian Crum added a comment -

        Scott,

        My suggestion is based on recent discussions about the widget models. From my perspective, it would be best if they were immutable. So, let's imagine they were. How would you modify an immutable model? You could decorate it. Get the existing model, and wrap it with a new one that has additional stuff in it. Then tell the factory to use your decorated model instead of the original one. Other applications could do the same thing to decorate your decorator, etc.

        Show
        Adrian Crum added a comment - Scott, My suggestion is based on recent discussions about the widget models. From my perspective, it would be best if they were immutable. So, let's imagine they were. How would you modify an immutable model? You could decorate it. Get the existing model, and wrap it with a new one that has additional stuff in it. Then tell the factory to use your decorated model instead of the original one. Other applications could do the same thing to decorate your decorator, etc.
        Hide
        Scott Gray added a comment -

        A few points:

        • I'm little vague on how immutability works but keep in mind that the model is modified during construction only.
        • I wrote this patch before that discussion was had.
        • I actually couldn't care less about how the functionality is implemented, it is the functionality itself that is important to me.
        • This patch is just a draft of the functionality that I envisaged, people are free to do with it as they please. The only thing I ask is that any changes to the actual plug-in behavior be discussed thoroughly.
        • If the base model is never actually used because the factory always returns the injected version then to be honest I don't see what benefits are gained by not touching it directly.
        Show
        Scott Gray added a comment - A few points: I'm little vague on how immutability works but keep in mind that the model is modified during construction only. I wrote this patch before that discussion was had. I actually couldn't care less about how the functionality is implemented, it is the functionality itself that is important to me. This patch is just a draft of the functionality that I envisaged, people are free to do with it as they please. The only thing I ask is that any changes to the actual plug-in behavior be discussed thoroughly. If the base model is never actually used because the factory always returns the injected version then to be honest I don't see what benefits are gained by not touching it directly.
        Hide
        Scott Gray added a comment -

        Sorry, in hindsight I probably shouldn't have bothered with the last point, it really makes no difference to me how we come to an inject menu model.

        Show
        Scott Gray added a comment - Sorry, in hindsight I probably shouldn't have bothered with the last point, it really makes no difference to me how we come to an inject menu model.
        Hide
        Bruno Busco added a comment -

        Hi Scott,
        installed the patches.
        The menu injection works well, I can see the new item in the Catalog menu.
        It seems that I cannot have the controller injections working since if I choose the "Google base integration" menu item I get the error:
        org.ofbiz.webapp.control.RequestHandlerException: Unknown request [gbaseadvancedsearch]; this request does not exist or cannot be called directly.

        Show
        Bruno Busco added a comment - Hi Scott, installed the patches. The menu injection works well, I can see the new item in the Catalog menu. It seems that I cannot have the controller injections working since if I choose the "Google base integration" menu item I get the error: org.ofbiz.webapp.control.RequestHandlerException: Unknown request [gbaseadvancedsearch] ; this request does not exist or cannot be called directly.
        Hide
        Scott Gray added a comment -

        Bruno,

        Hmm that is strange, I was sure it was all working at the time and I doubt anything has changed in the framework that would have caused it to stop working. I really don't have time to look into it right now but I'm sure whatever the problem is it could be solved with a minimal amount of debugging. The controller injections are much simpler than than the menu injections.

        When constructed the controller just asks the component config class for any injections and then loads them in, that's really all there is to it.

        Show
        Scott Gray added a comment - Bruno, Hmm that is strange, I was sure it was all working at the time and I doubt anything has changed in the framework that would have caused it to stop working. I really don't have time to look into it right now but I'm sure whatever the problem is it could be solved with a minimal amount of debugging. The controller injections are much simpler than than the menu injections. When constructed the controller just asks the component config class for any injections and then loads them in, that's really all there is to it.
        Hide
        Bruno Busco added a comment -

        Thank you Scott,
        I will find out.

        Show
        Bruno Busco added a comment - Thank you Scott, I will find out.
        Hide
        Bruno Busco added a comment -

        I am sorry to say that I was not able to fix this.
        Could someone interested, and more expert than me with java, try to fix the patch so that could be committed?
        I think it will definitively help in the framework separation.

        Thank you,
        Bruno

        Show
        Bruno Busco added a comment - I am sorry to say that I was not able to fix this. Could someone interested, and more expert than me with java, try to fix the patch so that could be committed? I think it will definitively help in the framework separation. Thank you, Bruno
        Hide
        Scott Gray added a comment -

        I'll try and find some time today to at least get it working again. That won't make it commit ready though, the roughness I originally mentioned will still be there.

        Show
        Scott Gray added a comment - I'll try and find some time today to at least get it working again. That won't make it commit ready though, the roughness I originally mentioned will still be there.
        Hide
        Scott Gray added a comment -

        There are two problems:

        1. There is a typo in the googlebase ofbiz-component file, three slashes instead of two for the google base controller location
        2. Adam made a change to the way URLs are returned from a call to ConfigXMLReader.getControllerConfigUrl(...) in r948439 which means I can no longer use the absolute file path as the key for retrieving injections. I haven't figured out a way around it yet and won't have the time to look at it again for a while.
        Show
        Scott Gray added a comment - There are two problems: There is a typo in the googlebase ofbiz-component file, three slashes instead of two for the google base controller location Adam made a change to the way URLs are returned from a call to ConfigXMLReader.getControllerConfigUrl(...) in r948439 which means I can no longer use the absolute file path as the key for retrieving injections. I haven't figured out a way around it yet and won't have the time to look at it again for a while.
        Hide
        Adam Heath added a comment -

        The url returned by ServletContext.getResource() can not be used outside of the servlet context that it is associated with. This is why catalina can return those jndi type urls. You may think that it works fine in catalina, but other containers can do different things.

        Also, ServletContext.getRealPath() can return null.

        Show
        Adam Heath added a comment - The url returned by ServletContext.getResource() can not be used outside of the servlet context that it is associated with. This is why catalina can return those jndi type urls. You may think that it works fine in catalina, but other containers can do different things. Also, ServletContext.getRealPath() can return null.
        Hide
        Scott Gray added a comment -

        Here's a somewhat hackish but working patch. Not intended for commit but useful for trying the injections stuff.

        Show
        Scott Gray added a comment - Here's a somewhat hackish but working patch. Not intended for commit but useful for trying the injections stuff.
        Hide
        Bruno Busco added a comment -

        Scott,
        what you think it should be done better in the patch to be committed?

        Show
        Bruno Busco added a comment - Scott, what you think it should be done better in the patch to be committed?
        Hide
        Scott Gray added a comment -

        Not sure
        I sort of threw it together as a proof of concept without putting much effort into code quality or naming conventions.

        Either this weekend coming or the one after I'll brush it off, give it another look and hopefully a tidy up, I do want to get it committed sometime soon. Thanks for the reminder

        Show
        Scott Gray added a comment - Not sure I sort of threw it together as a proof of concept without putting much effort into code quality or naming conventions. Either this weekend coming or the one after I'll brush it off, give it another look and hopefully a tidy up, I do want to get it committed sometime soon. Thanks for the reminder
        Hide
        Bruno Busco added a comment -

        That's great!
        Once commited I will try to use the new feature to get more components independence.

        Show
        Bruno Busco added a comment - That's great! Once commited I will try to use the new feature to get more components independence.
        Hide
        Bruno Busco added a comment -

        Hi Scott,
        is there something I can do to help finalizing this?

        Thank you,
        Bruno

        Show
        Bruno Busco added a comment - Hi Scott, is there something I can do to help finalizing this? Thank you, Bruno
        Hide
        Jacques Le Roux added a comment -

        Hi,

        Apart Scott, did someone tried the injection stuff?

        Show
        Jacques Le Roux added a comment - Hi, Apart Scott, did someone tried the injection stuff?

          People

          • Assignee:
            Unassigned
            Reporter:
            Bruno Busco
          • Votes:
            1 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:

              Development