Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: SVN trunk
    • Fix Version/s: SVN trunk
    • Component/s: framework
    • Labels:
      None

      Description

      The User Preferences Services are intended to store and retrieve the current user's preferences. User preferences can be used to control UI elements, frequently used search and report criteria, and more.

      1. COLLAPSD.JPG
        67 kB
        Adrian Crum
      2. example.patch
        13 kB
        Adrian Crum
      3. example.patch
        12 kB
        Adrian Crum
      4. example.patch
        12 kB
        Adrian Crum
      5. example.patch
        13 kB
        Adrian Crum
      6. EXPANDED.JPG
        78 kB
        Adrian Crum
      7. user_pref.patch
        14 kB
        Shubham Goyal
      8. user_pref.patch
        14 kB
        Anurag Meshram
      9. user_pref.patch
        32 kB
        Adrian Crum
      10. user_pref.patch
        32 kB
        Shankar Soni
      11. user_pref.patch
        37 kB
        Adrian Crum
      12. user_pref.patch
        36 kB
        Adrian Crum
      13. user_pref.patch
        35 kB
        Adrian Crum
      14. user_pref.patch
        40 kB
        Adrian Crum

        Activity

        Hide
        Jacques Le Roux added a comment -

        Adrian said that it's already in the Framework

        Show
        Jacques Le Roux added a comment - Adrian said that it's already in the Framework
        Hide
        Shubham Goyal added a comment -

        As Jacques reported the error, I have resolved it with the help of Pankaj Savita. Updated patch(user_pref.patch) has been attached.

        Show
        Shubham Goyal added a comment - As Jacques reported the error, I have resolved it with the help of Pankaj Savita. Updated patch(user_pref.patch) has been attached.
        Hide
        Jacques Le Roux added a comment - - edited

        Just tried last Anurag's patches. After an ant clean-all on a clean instance and just patches applied I got when trying
        https://127.0.0.1:8443/example/control/UserPrefDemo :

        Error on line 43, column 1 in component://example/webapp/example/viewDemo.ftl resultMap.userPrefMap is undefined. It cannot be assigned to defaultPrefMap The problematic instruction: ---------- ==> assignment: defaultPrefMap=resultMap.userPrefMap [on line 43, column 1 in component://example/webapp/example/viewDemo.ftl]

        I then tried in UserPreference to add DEFAULT_USER_PREFS by admin and NA without success. I then reverted framework/common/servicedef/services.xml and it worked : needs more work...

        (sorry typo in Anurag 1st name)

        Show
        Jacques Le Roux added a comment - - edited Just tried last Anurag's patches. After an ant clean-all on a clean instance and just patches applied I got when trying https://127.0.0.1:8443/example/control/UserPrefDemo : Error on line 43, column 1 in component://example/webapp/example/viewDemo.ftl resultMap.userPrefMap is undefined. It cannot be assigned to defaultPrefMap The problematic instruction: ---------- ==> assignment: defaultPrefMap=resultMap.userPrefMap [on line 43, column 1 in component://example/webapp/example/viewDemo.ftl] I then tried in UserPreference to add DEFAULT_USER_PREFS by admin and NA without success. I then reverted framework/common/servicedef/services.xml and it worked : needs more work... (sorry typo in Anurag 1st name)
        Hide
        Jacques Le Roux added a comment -

        Adrian,

        Yes sure.

        Show
        Jacques Le Roux added a comment - Adrian, Yes sure.
        Hide
        Adrian Crum added a comment -

        Jacques,

        The simple methods would replace the PreferenceServices.java file only. We still need the PreferenceWorker class.

        Show
        Adrian Crum added a comment - Jacques, The simple methods would replace the PreferenceServices.java file only. We still need the PreferenceWorker class.
        Hide
        Jacques Le Roux added a comment -

        Thanks for your work Anurag,

        I think we should replace (hence remove) all the the java part. Everybody agree ?

        Show
        Jacques Le Roux added a comment - Thanks for your work Anurag, I think we should replace (hence remove) all the the java part. Everybody agree ?
        Hide
        Anurag Meshram added a comment -

        I have reviwed the patch attached along with JIRA issue. I have done some changes in existing code and tested on Example component mentioned in "example.patch", It is working fine.
        Please have look on "user_pref.patch" for the further changes made in this review.

        Show
        Anurag Meshram added a comment - I have reviwed the patch attached along with JIRA issue. I have done some changes in existing code and tested on Example component mentioned in "example.patch", It is working fine. Please have look on "user_pref.patch" for the further changes made in this review.
        Hide
        Adrian Crum added a comment -

        Jacopo,

        I'm sure there are things that need to be built out in this new feature, and we'll just have to see how people use it.

        Regarding the primary key for the UserPreference entity: in order for the key to be unique, it would have to include the userPrefGroupId field, but that field is optional, so I left the primary key out of the entity. Most of the retrieval operations on this entity will be findByAnd, so I didn't think a primary key was necessary.

        Regarding field type differences between the two entities: userPrefGroupId is optional in the UserPreference entity, and it is required in the UserPrefGroupType entity. I didn't want to force developers to create a UserPrefGroupType record for every preference setting. I pictured the UserPrefGroupType entity being used primarily for the UI - where you need to describe to the user what the preference setting is used for. In other words, the User Preferences feature could operate just as well without the UserPrefGroupType entity.

        Show
        Adrian Crum added a comment - Jacopo, I'm sure there are things that need to be built out in this new feature, and we'll just have to see how people use it. Regarding the primary key for the UserPreference entity: in order for the key to be unique, it would have to include the userPrefGroupId field, but that field is optional, so I left the primary key out of the entity. Most of the retrieval operations on this entity will be findByAnd, so I didn't think a primary key was necessary. Regarding field type differences between the two entities: userPrefGroupId is optional in the UserPreference entity, and it is required in the UserPrefGroupType entity. I didn't want to force developers to create a UserPrefGroupType record for every preference setting. I pictured the UserPrefGroupType entity being used primarily for the UI - where you need to describe to the user what the preference setting is used for. In other words, the User Preferences feature could operate just as well without the UserPrefGroupType entity.
        Hide
        Jacopo Cappellato added a comment -

        I think there are a few things that should be fixed at least in the entity definitions: verify if primary keys are correctly set (I added the one for UserPreference that was missing), set foreign keys, the field type of UserPrefGroupType.userPrefGroupId is different from the one of UserPreference.userPrefGroupId and in general, we could verify if the naming convantions used for the fields are standard.

        Show
        Jacopo Cappellato added a comment - I think there are a few things that should be fixed at least in the entity definitions: verify if primary keys are correctly set (I added the one for UserPreference that was missing), set foreign keys, the field type of UserPrefGroupType.userPrefGroupId is different from the one of UserPreference.userPrefGroupId and in general, we could verify if the naming convantions used for the fields are standard.
        Hide
        Adrian Crum added a comment -

        Updated example.patch file.

        Show
        Adrian Crum added a comment - Updated example.patch file.
        Hide
        Adrian Crum added a comment -

        Java version committed - rev 606269.

        Show
        Adrian Crum added a comment - Java version committed - rev 606269.
        Hide
        BJ Freeman added a comment -

        Oh sorry. yes put in examples for proof of concept.
        +1

        Show
        BJ Freeman added a comment - Oh sorry. yes put in examples for proof of concept. +1
        Hide
        BJ Freeman added a comment -

        will start review after the first of the year.

        Show
        BJ Freeman added a comment - will start review after the first of the year.
        Hide
        Jacques Le Roux added a comment -

        +1 for me

        Show
        Jacques Le Roux added a comment - +1 for me
        Hide
        Adrian Crum added a comment -

        Jacopo,

        Maybe we could commit the Java version and leave this issue open. When the simple-method version is finished, we can commit that and close the issue. What do you think?

        Show
        Adrian Crum added a comment - Jacopo, Maybe we could commit the Java version and leave this issue open. When the simple-method version is finished, we can commit that and close the issue. What do you think?
        Hide
        Jacopo Cappellato added a comment -

        I must admin that I've never really reviewed this work, but I like the idea and I think that it could be used for other interesting things... for example as a basis to resolve the issue: https://issues.apache.org/jira/browse/OFBIZ-455

        Show
        Jacopo Cappellato added a comment - I must admin that I've never really reviewed this work, but I like the idea and I think that it could be used for other interesting things... for example as a basis to resolve the issue: https://issues.apache.org/jira/browse/OFBIZ-455
        Hide
        Adrian Crum added a comment -

        Updated example.patch to latest svn.

        Show
        Adrian Crum added a comment - Updated example.patch to latest svn.
        Hide
        Adrian Crum added a comment -

        Shankar Soni,

        Thank you for the patch! I tried using it on my local copy, but it doesn't work. I fixed a few things, but there are still problems with it. Please take another look at it. Also, use the example demo patch to test your work.

        Attached file contains my changes.

        Show
        Adrian Crum added a comment - Shankar Soni, Thank you for the patch! I tried using it on my local copy, but it doesn't work. I fixed a few things, but there are still problems with it. Please take another look at it. Also, use the example demo patch to test your work. Attached file contains my changes.
        Hide
        Shankar Soni added a comment -

        I have converted the PreferenceServices.java (Java Service) into PreferenceServices.xml (Simple method) and here I am uploading the new patch "user_pref.patch" with all the changes done till now.

        Show
        Shankar Soni added a comment - I have converted the PreferenceServices.java (Java Service) into PreferenceServices.xml (Simple method) and here I am uploading the new patch "user_pref.patch" with all the changes done till now.
        Hide
        Adrian Crum added a comment -

        Improved patch - added request map to common-controller.

        Show
        Adrian Crum added a comment - Improved patch - added request map to common-controller.
        Hide
        Jacopo Cappellato added a comment -

        David,
        please see my last comment to this issue.

        Show
        Jacopo Cappellato added a comment - David, please see my last comment to this issue.
        Hide
        Jacopo Cappellato added a comment -

        After a very cursory review,the last patch that Adrian created seems very clean (user_pref patch).
        David, would you mind giving another go? If not, I will assign it to me and, as soon as I have some free time I will test/commit it.
        In the meantime, to all OFBiz users/developers: please help to test it and put your votes here!!!

        Show
        Jacopo Cappellato added a comment - After a very cursory review,the last patch that Adrian created seems very clean (user_pref patch). David, would you mind giving another go? If not, I will assign it to me and, as soon as I have some free time I will test/commit it. In the meantime, to all OFBiz users/developers: please help to test it and put your votes here!!!
        Hide
        Adrian Crum added a comment -

        Updated example patch file. This file is NOT intended to be part of the project - it is here for testing and demo purposes only.

        Apply the patch, go to Webtools and import framework/example/data/UserPreferenceData.xml. Then navigate to:

        https://127.0.0.1:8443/example/control/UserPrefDemo

        to try out the demo.

        Show
        Adrian Crum added a comment - Updated example patch file. This file is NOT intended to be part of the project - it is here for testing and demo purposes only. Apply the patch, go to Webtools and import framework/example/data/UserPreferenceData.xml. Then navigate to: https://127.0.0.1:8443/example/control/UserPrefDemo to try out the demo.
        Hide
        Adrian Crum added a comment -

        Updated user_pref patch. Completely re-written. I moved the code from some of the additional files into the existing common component files - as David suggested. I renamed the new files that remained. The services use the new service permission style.

        I reorganized the methods in the two Java classes. The two classes combined total 500 or so lines, so if anyone wants to try to reduce them further by using minilang, they are welcome to do so. PreferenceServices.java would be the file to convert.

        Show
        Adrian Crum added a comment - Updated user_pref patch. Completely re-written. I moved the code from some of the additional files into the existing common component files - as David suggested. I renamed the new files that remained. The services use the new service permission style. I reorganized the methods in the two Java classes. The two classes combined total 500 or so lines, so if anyone wants to try to reduce them further by using minilang, they are welcome to do so. PreferenceServices.java would be the file to convert.
        Hide
        Adrian Crum added a comment -

        David and Jacques,

        Thank you very much for taking the time to review the patch. I will go through the files again to correct the formatting. The advice about new files and file naming was helpful.

        Jacques - I didn't update the example patch, so that is why you're getting errors. I will update the example to use the new code.

        Show
        Adrian Crum added a comment - David and Jacques, Thank you very much for taking the time to review the patch. I will go through the files again to correct the formatting. The advice about new files and file naming was helpful. Jacques - I didn't update the example patch, so that is why you're getting errors. I will update the example to use the new code.
        Hide
        Jacques Le Roux added a comment -

        Adrian, David,

        I agree with David about tabs (though there are not much tabs in this patch, only some in services_user.xml) but in .ftl where I far prefer 2 tabs (as Adrian did in example.patch, always not to be commited ?).

        I got a problem to patch controller.xml in example.patch but was able to merge it (thanks to the great Tortoise :o).

        Then I tested the link https://127.0.0.1:8443/example/control/UserPrefDemo but I got an error because location="$

        {parameters.mainDecoratorLocation}

        "> is missin in <screen name="UserPrefDemo">

        Then I got a problem
        Method public java.util.Map org.ofbiz.service.GenericDispatcher.runSync(java.lang.String,java.util.Map) throws org.ofbiz.service.ServiceValidationException,org.ofbiz.service.GenericServiceException threw an exception when invoked on org.ofbiz.service.WebAppDispatcher@9eb4a1 The problematic instruction: ---------- ==> assignment: resultMap=dispatcher.runSync("getUserPreference", Static["org.ofbiz.base.util.UtilMisc"].toMap("userPrefTypeId", "TEST_PREFERENCE", "userLogin", userLogin)) [on line 48, column 3 in component://example/webapp/example/viewDemo.ftl]

        because in UserPrefServices.getUserPreference userPrefGroupId was null when called in userPrefMap = UserPreferences.createUserPrefMap(UserPreferences.getUserPreference(delegator, (String) context.get("userLoginId"), userPrefTypeId, userPrefGroupId));

        Then I put "admin" in place of "NA" in UserServicesData.xml and re-imported : no way, I stopped there.

        Thanks for your work Adrian, just a little bit more and I guess it should be ok ;o)

        Also to mentio,n though not related (using Derby):
        2007-08-19 07:08:31,437 (main) [ DatabaseUtil.java:267:ERROR] WARNING: Column [TEXT_DATA] of table [OFBIZ.AGREEMENT] of entity [Agreement] is of type [VARCHAR] in the database, but is defined as type [CLOB] in the entity definition.
        2007-08-19 07:08:31,578 (main) [ DatabaseUtil.java:274:WARN ] WARNING: Column [TASK] of table [OFBIZ.OAGIS_MESSAGE_ERROR_INFO] of entity [OagisMessageErrorInfo] has a column size of [10] in the database, but is defined to have a column size of [60] in the entity definition.
        2007-08-19 07:08:31,578 (main) [ DatabaseUtil.java:267:ERROR] WARNING: Column [DESCRIPTION] of table [OFBIZ.OAGIS_MESSAGE_ERROR_INFO] of entity [OagisMessageErrorInfo] is of type [VARCHAR] in the database, but is defined as type [CLOB] in the entity definition.
        2007-08-19 07:08:31,578 (main) [ DatabaseUtil.java:274:WARN ] WARNING: Column [TASK] of table [OFBIZ.OAGIS_MESSAGE_INFO] of entity [OagisMessageInfo] has a column size of [10] in the database, but is defined to have a column size of [60] in the entity definition.

        Show
        Jacques Le Roux added a comment - Adrian, David, I agree with David about tabs (though there are not much tabs in this patch, only some in services_user.xml) but in .ftl where I far prefer 2 tabs (as Adrian did in example.patch, always not to be commited ?). I got a problem to patch controller.xml in example.patch but was able to merge it (thanks to the great Tortoise :o). Then I tested the link https://127.0.0.1:8443/example/control/UserPrefDemo but I got an error because location="$ {parameters.mainDecoratorLocation} "> is missin in <screen name="UserPrefDemo"> Then I got a problem Method public java.util.Map org.ofbiz.service.GenericDispatcher.runSync(java.lang.String,java.util.Map) throws org.ofbiz.service.ServiceValidationException,org.ofbiz.service.GenericServiceException threw an exception when invoked on org.ofbiz.service.WebAppDispatcher@9eb4a1 The problematic instruction: ---------- ==> assignment: resultMap=dispatcher.runSync("getUserPreference", Static ["org.ofbiz.base.util.UtilMisc"] .toMap("userPrefTypeId", "TEST_PREFERENCE", "userLogin", userLogin)) [on line 48, column 3 in component://example/webapp/example/viewDemo.ftl] because in UserPrefServices.getUserPreference userPrefGroupId was null when called in userPrefMap = UserPreferences.createUserPrefMap(UserPreferences.getUserPreference(delegator, (String) context.get("userLoginId"), userPrefTypeId, userPrefGroupId)); Then I put "admin" in place of " NA " in UserServicesData.xml and re-imported : no way, I stopped there. Thanks for your work Adrian, just a little bit more and I guess it should be ok ;o) Also to mentio,n though not related (using Derby): 2007-08-19 07:08:31,437 (main) [ DatabaseUtil.java:267:ERROR] WARNING: Column [TEXT_DATA] of table [OFBIZ.AGREEMENT] of entity [Agreement] is of type [VARCHAR] in the database, but is defined as type [CLOB] in the entity definition. 2007-08-19 07:08:31,578 (main) [ DatabaseUtil.java:274:WARN ] WARNING: Column [TASK] of table [OFBIZ.OAGIS_MESSAGE_ERROR_INFO] of entity [OagisMessageErrorInfo] has a column size of [10] in the database, but is defined to have a column size of [60] in the entity definition. 2007-08-19 07:08:31,578 (main) [ DatabaseUtil.java:267:ERROR] WARNING: Column [DESCRIPTION] of table [OFBIZ.OAGIS_MESSAGE_ERROR_INFO] of entity [OagisMessageErrorInfo] is of type [VARCHAR] in the database, but is defined as type [CLOB] in the entity definition. 2007-08-19 07:08:31,578 (main) [ DatabaseUtil.java:274:WARN ] WARNING: Column [TASK] of table [OFBIZ.OAGIS_MESSAGE_INFO] of entity [OagisMessageInfo] has a column size of [10] in the database, but is defined to have a column size of [60] in the entity definition.
        Hide
        David E. Jones added a comment -

        Adrian,

        I started reviewing this with the intent of committing it, but some of the issues I mentioned before still exist, and what is implemented here varies too much from best practices to be a good basis for what will likely live for a long time and be expanded and such quite a bit.

        1. 4 spaces, no tabs; also always 4 spaces and not 6, not 2
        2. don't create new files where not necessary, and where it varies from establish artifacts and practices, specifically the UserPrefMessages.properties, UserSecurityData.xml, and UserServicesData.xml
        3. name things using names that get to the point; for example, services_user.xml is not only not specific, it is misleading; the point of that file is preferences not users
        4. implement these data moving services using simple-methods rather than Java; based on a quick review the code will be about 1/4 the size
        5. if you are going to implement in Java, at least follow the Sun standard code formatting guidelines that we are trying to get everyone to use, as mentioned here: http://docs.ofbiz.org/display/OFBADMIN/Coding+Conventions

        After more experience with OFBiz I think you're probably aware of much of this, and I know that most of this is older code. One way or another this stuff needs to be cleaned up. Don't take it too personally, this sort of new addition is actually one of the easier types of things to review thoroughly.

        Show
        David E. Jones added a comment - Adrian, I started reviewing this with the intent of committing it, but some of the issues I mentioned before still exist, and what is implemented here varies too much from best practices to be a good basis for what will likely live for a long time and be expanded and such quite a bit. 1. 4 spaces, no tabs; also always 4 spaces and not 6, not 2 2. don't create new files where not necessary, and where it varies from establish artifacts and practices, specifically the UserPrefMessages.properties, UserSecurityData.xml, and UserServicesData.xml 3. name things using names that get to the point; for example, services_user.xml is not only not specific, it is misleading; the point of that file is preferences not users 4. implement these data moving services using simple-methods rather than Java; based on a quick review the code will be about 1/4 the size 5. if you are going to implement in Java, at least follow the Sun standard code formatting guidelines that we are trying to get everyone to use, as mentioned here: http://docs.ofbiz.org/display/OFBADMIN/Coding+Conventions After more experience with OFBiz I think you're probably aware of much of this, and I know that most of this is older code. One way or another this stuff needs to be cleaned up. Don't take it too personally, this sort of new addition is actually one of the easier types of things to review thoroughly.
        Hide
        Jacques Le Roux added a comment -

        I like the idea (no reviews, no tests yet). I suggest to create a box or something to put all stuffes in when expanded.

        Show
        Jacques Le Roux added a comment - I like the idea (no reviews, no tests yet). I suggest to create a box or something to put all stuffes in when expanded.
        Hide
        Adrian Crum added a comment -

        user_pref.patch is updated against the latest svn. I simplified a lot of code and the entities.

        You can specify a preference value's data type as a java data type now. I eliminated the need for an enumeration entity to specify the data type.

        The two screen shots show how I used the user preferences feature to implement a collapsible masthead.

        Show
        Adrian Crum added a comment - user_pref.patch is updated against the latest svn. I simplified a lot of code and the entities. You can specify a preference value's data type as a java data type now. I eliminated the need for an enumeration entity to specify the data type. The two screen shots show how I used the user preferences feature to implement a collapsible masthead.
        Hide
        Jacques Le Roux added a comment -

        Should we close this issue ?

        Show
        Jacques Le Roux added a comment - Should we close this issue ?
        Hide
        Adrian Crum added a comment -

        The updated example screen. Same instructions as before.

        This patch is NOT intended to be part of the main project. It is for demonstration purposes only.

        Show
        Adrian Crum added a comment - The updated example screen. Same instructions as before. This patch is NOT intended to be part of the main project. It is for demonstration purposes only.
        Hide
        Adrian Crum added a comment -

        An updated version of the User Preferences feature.

        David Jones suggested using simple methods for this, but I didn't convert it over due to lack of time. Several developers expressed a need for this feature, so I just updated it against the latest SVN and tested it.

        Prior patches can be ignored/deleted.

        Show
        Adrian Crum added a comment - An updated version of the User Preferences feature. David Jones suggested using simple methods for this, but I didn't convert it over due to lack of time. Several developers expressed a need for this feature, so I just updated it against the latest SVN and tested it. Prior patches can be ignored/deleted.
        Hide
        Adrian Crum added a comment -

        My apologies Jacques, the correct file name is framework/example/data/UserServicesData.xml. I confirmed that it is in the example.patch file.

        Show
        Adrian Crum added a comment - My apologies Jacques, the correct file name is framework/example/data/UserServicesData.xml. I confirmed that it is in the example.patch file.
        Hide
        Jacques Le Roux added a comment -

        Adrian,

        I applied and tried to try this patch (example.patch) but it seems that at least UserPrefData.xml is missing. I thought it maybe a mistake on file name and tried to import UserServicesData.xml (the only data file in this patch) but it did not work either. BTW there is no UserPrefData.xml in framework_v2.patch nor in framework.patch

        Show
        Jacques Le Roux added a comment - Adrian, I applied and tried to try this patch (example.patch) but it seems that at least UserPrefData.xml is missing. I thought it maybe a mistake on file name and tried to import UserServicesData.xml (the only data file in this patch) but it did not work either. BTW there is no UserPrefData.xml in framework_v2.patch nor in framework.patch
        Hide
        Adrian Crum added a comment -

        Uploaded example.patch:

        This patch contains the User Preference demo screen that was in the original framework.patch file. It is NOT intended to be committed to the project - it is for demonstration purposes only.

        Apply the patch to the framework/example folder. Start OFBiz and import the framework/example/data/UserPrefData.xml file using Webtools. Browse to

        https://127.0.0.1:8443/example/control/UserPrefDemo

        to view the demo page.

        Show
        Adrian Crum added a comment - Uploaded example.patch: This patch contains the User Preference demo screen that was in the original framework.patch file. It is NOT intended to be committed to the project - it is for demonstration purposes only. Apply the patch to the framework/example folder. Start OFBiz and import the framework/example/data/UserPrefData.xml file using Webtools. Browse to https://127.0.0.1:8443/example/control/UserPrefDemo to view the demo page.
        Hide
        Adrian Crum added a comment -

        Uploaded framework_v2.patch:

        The User Preferences feature has been updated to include David's suggestions.

        1.The "Java formatting faux pas" were in the original UtilMisc.java file, which I copy/pasted one method in. I reformatted the entire file to get it to comply with current OFBiz coding standards. At the very least, this file should be committed.

        2. The User Preference feature has been moved to the common component.

        3. I fixed the problems with my SVN client which cause duplicate files in the original patch.

        4. I eliminated the example component demo code and data.

        I applied the patch to the framework folder of the most recent SVN version and it works.

        I'm done with this issue. If anyone else wants to develop it further, they are welcome to do so.

        Show
        Adrian Crum added a comment - Uploaded framework_v2.patch: The User Preferences feature has been updated to include David's suggestions. 1.The "Java formatting faux pas" were in the original UtilMisc.java file, which I copy/pasted one method in. I reformatted the entire file to get it to comply with current OFBiz coding standards. At the very least, this file should be committed. 2. The User Preference feature has been moved to the common component. 3. I fixed the problems with my SVN client which cause duplicate files in the original patch. 4. I eliminated the example component demo code and data. I applied the patch to the framework folder of the most recent SVN version and it works. I'm done with this issue. If anyone else wants to develop it further, they are welcome to do so.
        Hide
        Adrian Crum added a comment -

        David,

        Thank you very much for reviewing this! I'll adress the formatting/patch issues after it has been discussed - I figured I will be rewriting sections after it has been evaluated. I wanted to get it out there ASAP so people could comment on it, and for some reason my SVN client started acting flakey.

        I don't know about the dozens of lines thing. Most of it I wrote from scratch. The only existing file that was modified (outside the example component) was UtilMisc. As far as I know I didn't reformat that. I'll take a look at it though.

        The Java classes were used so they can be subclassed or wrapped with other Java classes. I was trying to make it as flexible/extensible as possible.

        By the way, for anyone reviewing this - the example component is just being used to demonstrate the feature. That part of the patch is not intended to be made part of the project.

        Thanks again for taking a look at this. I'll address your suggestions once everyone has commented.

        Show
        Adrian Crum added a comment - David, Thank you very much for reviewing this! I'll adress the formatting/patch issues after it has been discussed - I figured I will be rewriting sections after it has been evaluated. I wanted to get it out there ASAP so people could comment on it, and for some reason my SVN client started acting flakey. I don't know about the dozens of lines thing. Most of it I wrote from scratch. The only existing file that was modified (outside the example component) was UtilMisc. As far as I know I didn't reformat that. I'll take a look at it though. The Java classes were used so they can be subclassed or wrapped with other Java classes. I was trying to make it as flexible/extensible as possible. By the way, for anyone reviewing this - the example component is just being used to demonstrate the feature. That part of the patch is not intended to be made part of the project. Thanks again for taking a look at this. I'll address your suggestions once everyone has commented.
        Hide
        David E. Jones added a comment -

        Just a few quick thoughts based on an initial review of this:

        1. there are dozens of lines unnecessarily changed in this patch (ie formatting changes, etc that have nothing to do with functionality)
        2. a few Java formatting faux pas, like if/else without curly braces
        3. this should go into an existing component instead of being added to a new component; perhaps the "common" component would be a good fit, but this should probably be discussed (along with various other things)
        4. why were the services written in Java and not as simple-methods? these seem to mostly be getting and organizing data into generic structures, which is what simple-methods are best for
        5. this patch somehow has two copies of each file/change...

        In general because this is a fair sized change this should be evaluated and feedback offered by various people. Review of this sort of thing should include review of entity model additions, security/permissions, seed and demo data (and properly split into seed and demo files), implementation approaches, general organization, examples, etc.

        More generally, this looks like a great start Adrian, so I hope it picks up interest and is finished and included in OFBiz.

        Show
        David E. Jones added a comment - Just a few quick thoughts based on an initial review of this: 1. there are dozens of lines unnecessarily changed in this patch (ie formatting changes, etc that have nothing to do with functionality) 2. a few Java formatting faux pas, like if/else without curly braces 3. this should go into an existing component instead of being added to a new component; perhaps the "common" component would be a good fit, but this should probably be discussed (along with various other things) 4. why were the services written in Java and not as simple-methods? these seem to mostly be getting and organizing data into generic structures, which is what simple-methods are best for 5. this patch somehow has two copies of each file/change... In general because this is a fair sized change this should be evaluated and feedback offered by various people. Review of this sort of thing should include review of entity model additions, security/permissions, seed and demo data (and properly split into seed and demo files), implementation approaches, general organization, examples, etc. More generally, this looks like a great start Adrian, so I hope it picks up interest and is finished and included in OFBiz.
        Hide
        Adrian Crum added a comment -

        The patch.

        Show
        Adrian Crum added a comment - The patch.

          People

          • Assignee:
            Jacques Le Roux
            Reporter:
            Adrian Crum
          • Votes:
            3 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development