Uploaded image for project: 'OFBiz'
  1. OFBiz
  2. OFBIZ-9123

Add a PriCat component under specialpurpose

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Trivial
    • Resolution: Works for Me
    • Affects Version/s: Trunk
    • Fix Version/s: Trunk
    • Component/s: None
    • Labels:
      None

      Description

      Add a PriCat component to view excel import with html report and download excel output with errors commented.

        Activity

        Hide
        shi.jinghai Shi Jinghai added a comment -

        The pricat component is in trunk rev. 1770621 and 1770622.

        Have fun.

        Show
        shi.jinghai Shi Jinghai added a comment - The pricat component is in trunk rev. 1770621 and 1770622. Have fun.
        Hide
        mbrohl Michael Brohl added a comment -

        Hi Jinghai,

        thanks for your contribution. It's appreciated and I am not against putting it into the repository but I think we should at least

        1. have a discussion if it should be added and
        2. let someone review it before it is added.

        We want to stabilize and slim down the codebase so we should follow some review process.

        Maybe I missed some discussion about it but cannot find some under the keyword "pricat".

        Show
        mbrohl Michael Brohl added a comment - Hi Jinghai, thanks for your contribution. It's appreciated and I am not against putting it into the repository but I think we should at least 1. have a discussion if it should be added and 2. let someone review it before it is added. We want to stabilize and slim down the codebase so we should follow some review process. Maybe I missed some discussion about it but cannot find some under the keyword "pricat".
        Hide
        shi.jinghai Shi Jinghai added a comment -

        Hi Michael,

        Honestly, this piece of pricat component has been there over 10 years, not new concept.

        Sorry, my English is not good enough, what's "slim down" mean? Personally, I thought the gradle effort is done for the slim purpose.

        Show
        shi.jinghai Shi Jinghai added a comment - Hi Michael, Honestly, this piece of pricat component has been there over 10 years, not new concept. Sorry, my English is not good enough, what's "slim down" mean? Personally, I thought the gradle effort is done for the slim purpose.
        Hide
        mbrohl Michael Brohl added a comment -

        Maybe I have not expressed myself well enough.

        If I got it right, you have added a new component under specialpurpose and I cannot see that it was there before or that it was moved from the applications to specialpurpose. My point is not that you've added a new component but the chosen process of doing so. As far as I can see you created this Jira and immediately committed your work to trunk without any discussion or review before.

        In my opinion the process should be:

        1. create a Jira with a good description of the contribution, what does it do, why should it be added etc.
        2. provide a patch and ask for review
        3. review and discussion by the community
        4. if the community wants to add the contribution, it can be committed.

        Even if we as committers have direct access to the repository, we should still follow the process of contribute - discuss - review - commit as any other contributor.

        To be clear, I have not reviewed the contribution yet so I have currently no opinion if we should add it to the repository.

        Show
        mbrohl Michael Brohl added a comment - Maybe I have not expressed myself well enough. If I got it right, you have added a new component under specialpurpose and I cannot see that it was there before or that it was moved from the applications to specialpurpose. My point is not that you've added a new component but the chosen process of doing so. As far as I can see you created this Jira and immediately committed your work to trunk without any discussion or review before. In my opinion the process should be: 1. create a Jira with a good description of the contribution, what does it do, why should it be added etc. 2. provide a patch and ask for review 3. review and discussion by the community 4. if the community wants to add the contribution, it can be committed. Even if we as committers have direct access to the repository, we should still follow the process of contribute - discuss - review - commit as any other contributor. To be clear, I have not reviewed the contribution yet so I have currently no opinion if we should add it to the repository.
        Hide
        pfm.smits Pierre Smits added a comment -

        I already did a cursory review and found that the component is not in line with recent established best practices. Please revert, and upload as patch files to this issue.

        Show
        pfm.smits Pierre Smits added a comment - I already did a cursory review and found that the component is not in line with recent established best practices. Please revert, and upload as patch files to this issue.
        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        Hi Jinghai,

        I concur with Michael's opinion, notably the well stated 4 points process.

        After a cursory review, I did not find any documentation, either as a README or in the wiki. I have also run the pricatdemo and got what seems to be working results but I have not ideas of what is expected. I then ran pricat and got only this message "No excel import history data".

        Pricat does not seem to need much documentation but a minimum is expected

        I did not a thorough review, but globaly this is very simple and seems well done, so for me it's only lacking documentation. At this stage I'd not ask to revert.

        Now I'm more and more thinking about plugins and specialpurpose. When the pushPlugin Gradle task will be completed (not only in localhost), I'd prefer to have plugins available in an OFBiz common Maven repo (on our demo VM for instance). Then some of the current specialpurpose components and new components plugins would be maintained by their contributors, pricat would be a good candidate for that.

        We could use something like what Pierre proposes with https://oem.ofbizci.net/oci-2/ (currently inaccessible with Firefox due to HSTS policy, but OK with Chrome) to promote those plugins. It would be good if we could have Pierre's work as a common OFBiz feature, again on our demo VM. The idea is to have the OFBiz project keeping control over this aspect. If a such idea gets support would you contribute your work to the OFBiz project Pierre? For now I start the discussion here, but if it gets more interest I'll start it again on the dev ML.

        Show
        jacques.le.roux Jacques Le Roux added a comment - Hi Jinghai, I concur with Michael's opinion, notably the well stated 4 points process. After a cursory review, I did not find any documentation, either as a README or in the wiki. I have also run the pricatdemo and got what seems to be working results but I have not ideas of what is expected. I then ran pricat and got only this message "No excel import history data". Pricat does not seem to need much documentation but a minimum is expected I did not a thorough review, but globaly this is very simple and seems well done, so for me it's only lacking documentation. At this stage I'd not ask to revert. Now I'm more and more thinking about plugins and specialpurpose. When the pushPlugin Gradle task will be completed (not only in localhost), I'd prefer to have plugins available in an OFBiz common Maven repo (on our demo VM for instance). Then some of the current specialpurpose components and new components plugins would be maintained by their contributors, pricat would be a good candidate for that. We could use something like what Pierre proposes with https://oem.ofbizci.net/oci-2/ (currently inaccessible with Firefox due to HSTS policy, but OK with Chrome) to promote those plugins. It would be good if we could have Pierre's work as a common OFBiz feature, again on our demo VM. The idea is to have the OFBiz project keeping control over this aspect. If a such idea gets support would you contribute your work to the OFBiz project Pierre? For now I start the discussion here, but if it gets more interest I'll start it again on the dev ML.
        Hide
        shi.jinghai Shi Jinghai added a comment -

        Hi Jacques,

        Thanks for your review!

        I'll add a readme asap, and remove tabs in the source code.

        I'm eager to see the plugin model works in OFBiz. I have publish right of central maven repository but it's not allowed to publish any artifacts of Apache as Apache has ourown repository server, my question is where to publish our plugins?

        Love you,

        Show
        shi.jinghai Shi Jinghai added a comment - Hi Jacques, Thanks for your review! I'll add a readme asap, and remove tabs in the source code. I'm eager to see the plugin model works in OFBiz. I have publish right of central maven repository but it's not allowed to publish any artifacts of Apache as Apache has ourown repository server, my question is where to publish our plugins? Love you,
        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        Hi Jinghai,

        My proposition is to create a Maven repository in our demo server (ie at https://ofbiz-vm2.apache.org). We will certainly need to ask for more RAM (we run the 3 demos in 4GB) but I expect the infra to agree. The idea is to have something centralised that we can check, control and recommend.

        I thought about my question to Pierre, maybe something like that is not needed (at least for now) and a simple page in wiki linked from main site would be enough. There we would list the available plugins with simple (and possibly more elaborate) explanations about what they add and on how to proceed to install them from the (hopefully to come) OFBiz Maven repo.

        Show
        jacques.le.roux Jacques Le Roux added a comment - Hi Jinghai, My proposition is to create a Maven repository in our demo server (ie at https://ofbiz-vm2.apache.org ). We will certainly need to ask for more RAM (we run the 3 demos in 4GB) but I expect the infra to agree. The idea is to have something centralised that we can check, control and recommend. I thought about my question to Pierre, maybe something like that is not needed (at least for now) and a simple page in wiki linked from main site would be enough. There we would list the available plugins with simple (and possibly more elaborate) explanations about what they add and on how to proceed to install them from the (hopefully to come) OFBiz Maven repo.
        Hide
        shi.jinghai Shi Jinghai added a comment -

        Hi Jacques,

        I submitted a simple readme and replaced some (many) tabs with spaces.

        BTW, Apache Archiva is a good tool to build a maven repo.

        Show
        shi.jinghai Shi Jinghai added a comment - Hi Jacques, I submitted a simple readme and replaced some (many) tabs with spaces. BTW, Apache Archiva is a good tool to build a maven repo.
        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        Thanks for the tip Jinghai.

        Show
        jacques.le.roux Jacques Le Roux added a comment - Thanks for the tip Jinghai.
        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        Actually you did not submit but commit

        Show
        jacques.le.roux Jacques Le Roux added a comment - Actually you did not submit but commit
        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        Hi Jinghai,

        IIRW it has been already discussed but I can't find where (maybe in HipChat where history is not the best part). Could you please refresh my mind about why OFBizPricatUtil class must be in org.apache.poi.xssf.usermodel package? Thanks

        Show
        jacques.le.roux Jacques Le Roux added a comment - Hi Jinghai, IIRW it has been already discussed but I can't find where (maybe in HipChat where history is not the best part). Could you please refresh my mind about why OFBizPricatUtil class must be in org.apache.poi.xssf.usermodel package? Thanks
        Hide
        pfm.smits Pierre Smits added a comment - - edited

        Jacques,

        I wonder what it is that you want to have the project having control over.

        Show
        pfm.smits Pierre Smits added a comment - - edited Jacques, I wonder what it is that you want to have the project having control over.
        Hide
        pfm.smits Pierre Smits added a comment -

        Following up on this the freemarker templates are not in the correct place. As per recently revisited best practices these should not reside in the WEB-INF sub folder.

        Show
        pfm.smits Pierre Smits added a comment - Following up on this the freemarker templates are not in the correct place. As per recently revisited best practices these should not reside in the WEB-INF sub folder.
        Hide
        shi.jinghai Shi Jinghai added a comment -

        Hi Jacques,

        Here's the test case on OFBizPricatUtil:
        1. Import the sample pricat file and download it;
        2. Comment out the OFBizPricatUtil.formatCommentShape method used in pricat;
        3. Rebuild and import the sample pricat file;
        4. Download the commented pricat file;
        5. Open it in Microsoft Excel, the style of comments is not readable.
        6. Open it in OpenOffice or WPS, the style of comments is correct.

        Unzip the commented pricat file (.xlsx), compare the files with/without running OFBizPricatUtil.formatCommentShape, the difference is in xl/drawings/vmlDrawing1.vml file. According to POI document:

        In Excel 2007 VML drawings are used to describe properties of cell comments, although the spec says that VML is deprecated:
        The VML format is a legacy format originally introduced with Office 2000 and is included and fully defined in this Standard for backwards compatibility reasons. The DrawingML format is a newer and richer format created with the goal of eventually replacing any uses of VML in the Office Open XML formats. VML should be considered a deprecated format included in Office Open XML for legacy reasons only and new applications that need a file format for drawings are strongly encouraged to use preferentially DrawingML

        Warning - Excel is known to put invalid XML into these files! For example, >br< without being closed or escaped crops up.
        See 6.4 VML - SpreadsheetML Drawing in Office Open XML Part 4 - Markup Language Reference.pdf

        By putting OFBizPricatUtil under POI path, I can call the protected method XSSFVMLDrawing.findCommentShape and build the comments look right in Microsoft Excel.

        This is the reason. I'm sure there's a better way to do this.

        Kind Regards,

        Show
        shi.jinghai Shi Jinghai added a comment - Hi Jacques, Here's the test case on OFBizPricatUtil: 1. Import the sample pricat file and download it; 2. Comment out the OFBizPricatUtil.formatCommentShape method used in pricat; 3. Rebuild and import the sample pricat file; 4. Download the commented pricat file; 5. Open it in Microsoft Excel, the style of comments is not readable. 6. Open it in OpenOffice or WPS, the style of comments is correct. Unzip the commented pricat file (.xlsx), compare the files with/without running OFBizPricatUtil.formatCommentShape, the difference is in xl/drawings/vmlDrawing1.vml file. According to POI document: In Excel 2007 VML drawings are used to describe properties of cell comments, although the spec says that VML is deprecated: The VML format is a legacy format originally introduced with Office 2000 and is included and fully defined in this Standard for backwards compatibility reasons. The DrawingML format is a newer and richer format created with the goal of eventually replacing any uses of VML in the Office Open XML formats. VML should be considered a deprecated format included in Office Open XML for legacy reasons only and new applications that need a file format for drawings are strongly encouraged to use preferentially DrawingML Warning - Excel is known to put invalid XML into these files! For example, >br< without being closed or escaped crops up. See 6.4 VML - SpreadsheetML Drawing in Office Open XML Part 4 - Markup Language Reference.pdf By putting OFBizPricatUtil under POI path, I can call the protected method XSSFVMLDrawing.findCommentShape and build the comments look right in Microsoft Excel. This is the reason. I'm sure there's a better way to do this. Kind Regards,
        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        Thanks for your comment Pierre.

        Indeed Jinghai, I understand this is a contribution from an old work, but please change the location of the freemarker templates like in other components.

        For the css files I'd say it's fine for now. We have still 3 types (names) of location: includes, images or static. We already discussed that and I think we concluded static is better or maybe resources rather. Anyway if we change that it should be done everywhere.

        Show
        jacques.le.roux Jacques Le Roux added a comment - Thanks for your comment Pierre. Indeed Jinghai, I understand this is a contribution from an old work, but please change the location of the freemarker templates like in other components. For the css files I'd say it's fine for now. We have still 3 types (names) of location: includes, images or static. We already discussed that and I think we concluded static is better or maybe resources rather. Anyway if we change that it should be done everywhere.
        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        Thanks for the explanation Jinghai. So let's wait Microsoft

        Show
        jacques.le.roux Jacques Le Roux added a comment - Thanks for the explanation Jinghai. So let's wait Microsoft
        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        Just for what is already OOTB, like pricat in specialpurpose. So would be all or most of specialpurpose components. They would be available as plugins and not in the svn repo. They would be maintained by their contributors. It opens more possibilities for contributors wanting to donate their code to OFBiz while not putting all the burden on the committers shoulders once donated. More an idea still needing discussion for now...

        Show
        jacques.le.roux Jacques Le Roux added a comment - Just for what is already OOTB, like pricat in specialpurpose. So would be all or most of specialpurpose components. They would be available as plugins and not in the svn repo. They would be maintained by their contributors. It opens more possibilities for contributors wanting to donate their code to OFBiz while not putting all the burden on the committers shoulders once donated. More an idea still needing discussion for now...
        Hide
        pfm.smits Pierre Smits added a comment -

        Huh?

        Committers are required to commit changes for the various OFBiz products....

        Show
        pfm.smits Pierre Smits added a comment - Huh? Committers are required to commit changes for the various OFBiz products....
        Hide
        shi.jinghai Shi Jinghai added a comment -

        Ha, finally I got the feeling of what you are worrying about. When plugin done, I can take back the LDAP, Passport, Pricat and Solr components, move them to github.

        Show
        shi.jinghai Shi Jinghai added a comment - Ha, finally I got the feeling of what you are worrying about. When plugin done, I can take back the LDAP, Passport, Pricat and Solr components, move them to github.
        Hide
        pfm.smits Pierre Smits added a comment - - edited

        Hi Shi Jinghai,

        I have already made the following components (and others) available on github:

        All straight from trunk.

        Show
        pfm.smits Pierre Smits added a comment - - edited Hi Shi Jinghai , I have already made the following components (and others) available on github: Solr - https://github.com/OFBizCI/solr LDAP - https://github.com/OFBizCI/ldap Passport - https://github.com/OFBizCI/passport All straight from trunk.
        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        The idea for now is more to move them to the ASF Maven repo. Then it would be easy for users, using the plugin mechanism completed to work with Maven repos, to plug or unplug them. Now we can also wonder if it's a real benefit to the project. The maintenance burden would be passed from all the committers to the contributors. This have still to be discussed...

        Show
        jacques.le.roux Jacques Le Roux added a comment - The idea for now is more to move them to the ASF Maven repo. Then it would be easy for users, using the plugin mechanism completed to work with Maven repos, to plug or unplug them. Now we can also wonder if it's a real benefit to the project. The maintenance burden would be passed from all the committers to the contributors. This have still to be discussed...
        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        Pierre, I guess they are updated regularly (automatically) from the official OFBiz GitHub repo?

        BTW, what is the purpose? Does this not risk to introduce more entropy and confusion in the project?

        Show
        jacques.le.roux Jacques Le Roux added a comment - Pierre, I guess they are updated regularly (automatically) from the official OFBiz GitHub repo? BTW, what is the purpose? Does this not risk to introduce more entropy and confusion in the project?
        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        Required is maybe not the right word Eventually it will be a community's decision. Is that good for the project? Why? etc...

        Show
        jacques.le.roux Jacques Le Roux added a comment - Required is maybe not the right word Eventually it will be a community's decision. Is that good for the project? Why? etc...
        Hide
        mbrohl Michael Brohl added a comment -

        This discussion has nothing to do with the issue, please move it to the dev ML.

        Show
        mbrohl Michael Brohl added a comment - This discussion has nothing to do with the issue, please move it to the dev ML.
        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        Agreed Michael, this has already been started in the "[DISCUSSION] Defining an OFBiz Project Strategy" thread http://markmail.org/message/nc2sm7k5s3hlay33 but we need a new separate [DISCUSSION] thread to clarify the community's decision about it.

        Show
        jacques.le.roux Jacques Le Roux added a comment - Agreed Michael, this has already been started in the " [DISCUSSION] Defining an OFBiz Project Strategy" thread http://markmail.org/message/nc2sm7k5s3hlay33 but we need a new separate [DISCUSSION] thread to clarify the community's decision about it.
        Hide
        pfm.smits Pierre Smits added a comment -

        The complete list of OFBiz components available as separate GitHub repos has been listed in this: http://ofbiz.markmail.org/message/rebzohptaimdpyzi

        Show
        pfm.smits Pierre Smits added a comment - The complete list of OFBiz components available as separate GitHub repos has been listed in this: http://ofbiz.markmail.org/message/rebzohptaimdpyzi
        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        Hi Jinghai,

        I have just changed the Java files you committed for pricat to use the svn:native option.

        I guess you don't use the "official OFBIZ Subversion client configuration file" as requested here
        https://cwiki.apache.org/confluence/display/OFBADMIN/OFBiz+Committers+Roles+and+Responsibilities#OFBizCommittersRolesandResponsibilities-CommittingChanges

        Please install it on your machine/s and remember to have it on new machines when necessary, thanks.

        Show
        jacques.le.roux Jacques Le Roux added a comment - Hi Jinghai, I have just changed the Java files you committed for pricat to use the svn:native option. I guess you don't use the "official OFBIZ Subversion client configuration file" as requested here https://cwiki.apache.org/confluence/display/OFBADMIN/OFBiz+Committers+Roles+and+Responsibilities#OFBizCommittersRolesandResponsibilities-CommittingChanges Please install it on your machine/s and remember to have it on new machines when necessary, thanks.
        Hide
        shi.jinghai Shi Jinghai added a comment -

        Thank you Jacques! You are my mentor forever

        No, I haven't installed the client configuration file. I'm installing it now.

        Have a nice day,

        Show
        shi.jinghai Shi Jinghai added a comment - Thank you Jacques! You are my mentor forever No, I haven't installed the client configuration file. I'm installing it now. Have a nice day,
        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        Forever I don't know, but thanks

        Show
        jacques.le.roux Jacques Le Roux added a comment - Forever I don't know, but thanks
        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        Hi Jinghai,

        We missed a point Pierre made:

        Following up on this the freemarker templates are not in the correct place. As per recently revisited best practices these should not reside in the WEB-INF sub folder.

        So pricat\webapp\pricat\ftl should be pricat\template

        Are there other points we missed Pierre, Michael?

        Show
        jacques.le.roux Jacques Le Roux added a comment - Hi Jinghai, We missed a point Pierre made: Following up on this the freemarker templates are not in the correct place. As per recently revisited best practices these should not reside in the WEB-INF sub folder. So pricat\webapp\pricat\ftl should be pricat\template Are there other points we missed Pierre, Michael?
        Hide
        shi.jinghai Shi Jinghai added a comment -

        Thank you Jacques for the reopen, and thanks Pierre and Michael for the code review and suggestion!

        I submitted the changes in rev. 1788744, please check if it's ok now.

        Kind Regards,

        Show
        shi.jinghai Shi Jinghai added a comment - Thank you Jacques for the reopen, and thanks Pierre and Michael for the code review and suggestion! I submitted the changes in rev. 1788744, please check if it's ok now. Kind Regards,
        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        Closing again

        Show
        jacques.le.roux Jacques Le Roux added a comment - Closing again

          People

          • Assignee:
            shi.jinghai Shi Jinghai
            Reporter:
            shi.jinghai Shi Jinghai
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development