Details

      Description

      I converted CRUD service to entity-auto for :
      PartyRole
      PostalAddressBoundary
      PartyClassification
      PartyClassificationGroup
      PartyAttribute
      Vendor
      PartyCarrierAccount

      Only entity with their services present on PartyServices.xml

      I change the PartyRole attributes service like that

      -        <attribute name="partyId" type="String" mode="IN" optional="true"/>
      -        <attribute name="roleTypeId" type="String" mode="IN" optional="false"/>
      +        <auto-attributes include="pk" mode="IN" optional="false"/>
      

      The currently simple method haven't a specific resolution if the partyId isn't pass, so the service failed with database constraint error.

      I run manual test from Party Profile screen with success (except for PostalAddressBoundary that haven't standard screen, so call by runService) and I run ./ant clean-all load-demo run-tests without error

      1. OFBIZ-5750.patch
        3 kB
        Taher Alkhateeb
      2. OFBIZ-5750.patch
        23 kB
        Nicolas Malin

        Activity

        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        Thanks Nicolas,

        your patch is in trunk at revision: 1622161

        After looking at
        OFBIZ-585
        http://svn.apache.org/viewvc/ofbiz/trunk/applications/party/src/org/ofbiz/party/party/PartyRoleServices.java?view=markup&pathrev=522473
        http://svn.apache.org/viewvc/ofbiz/trunk/applications/party/script/org/ofbiz/party/party/PartyServices.xml?r1=524899&r2=524898&pathrev=524899
        and content of ServiceUtil.getPartyIdCheckSecurity()

        I agree with you about createPartyRole and deletePartyRole, we can neglect having PARTYMGR_CREATE for creating/deleting a party role

        Show
        jacques.le.roux Jacques Le Roux added a comment - Thanks Nicolas, your patch is in trunk at revision: 1622161 After looking at OFBIZ-585 http://svn.apache.org/viewvc/ofbiz/trunk/applications/party/src/org/ofbiz/party/party/PartyRoleServices.java?view=markup&pathrev=522473 http://svn.apache.org/viewvc/ofbiz/trunk/applications/party/script/org/ofbiz/party/party/PartyServices.xml?r1=524899&r2=524898&pathrev=524899 and content of ServiceUtil.getPartyIdCheckSecurity() I agree with you about createPartyRole and deletePartyRole, we can neglect having PARTYMGR_CREATE for creating/deleting a party role
        Hide
        soledad Nicolas Malin added a comment -

        Ok, thanks for your review and comment Jacques.
        I will continue on next entities

        Show
        soledad Nicolas Malin added a comment - Ok, thanks for your review and comment Jacques. I will continue on next entities
        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        Reopen to check if no issues where introduced here (see OFBIZ-6703 for the case with PartyRole)

        Show
        jacques.le.roux Jacques Le Roux added a comment - Reopen to check if no issues where introduced here (see OFBIZ-6703 for the case with PartyRole)
        Hide
        taher Taher Alkhateeb added a comment -

        Hi Jacques,

        We need to reimplement createPartyRole. What is the preferred language for that? is it groovy, minilang or Java?

        Show
        taher Taher Alkhateeb added a comment - Hi Jacques, We need to reimplement createPartyRole. What is the preferred language for that? is it groovy, minilang or Java?
        Hide
        jacques.le.roux Jacques Le Roux added a comment - - edited

        For this kind of services (CRUD or almost) my preference is minilang (it was created for that), but others may have other opinions

        But wait, why do we need to reimplement createPartyRole, other cases like OFBIZ-6703? I was more thinking at the other changes done

        PostalAddressBoundary
        PartyClassification
        PartyClassificationGroup
        PartyAttribute
        Vendor
        PartyCarrierAccount

        but I (quickly) checked and found nothing to do for them.
        BTW, if it's necessary why not simply reuse what existed?

        Show
        jacques.le.roux Jacques Le Roux added a comment - - edited For this kind of services (CRUD or almost) my preference is minilang (it was created for that), but others may have other opinions But wait, why do we need to reimplement createPartyRole, other cases like OFBIZ-6703 ? I was more thinking at the other changes done PostalAddressBoundary PartyClassification PartyClassificationGroup PartyAttribute Vendor PartyCarrierAccount but I (quickly) checked and found nothing to do for them. BTW, if it's necessary why not simply reuse what existed?
        Hide
        taher Taher Alkhateeb added a comment -

        Hi Jacques,

        I agree we do not need to reimplement if we can just reverse the commit on OFBIZ-6703. The reason I asked is because I thought that maybe the general inclination for the community is to move away from minilang and this is why OFBIZ-6703 existed in the first place. I personally like minilang and find it very convenient as a simple DSL for basic CRUD

        Show
        taher Taher Alkhateeb added a comment - Hi Jacques, I agree we do not need to reimplement if we can just reverse the commit on OFBIZ-6703 . The reason I asked is because I thought that maybe the general inclination for the community is to move away from minilang and this is why OFBIZ-6703 existed in the first place. I personally like minilang and find it very convenient as a simple DSL for basic CRUD
        Hide
        jacques.le.roux Jacques Le Roux added a comment - - edited

        OK cool, of course we should not revert all the commit, only the createPartyRole, and we should be sure it's mandatory. Are they other cases like the one we resolved at OFBIZ-6703?

        I mean entity-auto are very convenient, but yes we should be careful with them, especially when replacing

        Show
        jacques.le.roux Jacques Le Roux added a comment - - edited OK cool, of course we should not revert all the commit, only the createPartyRole, and we should be sure it's mandatory. Are they other cases like the one we resolved at OFBIZ-6703 ? I mean entity-auto are very convenient, but yes we should be careful with them, especially when replacing
        Hide
        taher Taher Alkhateeb added a comment -

        Hi Jacques,

        I've added a patch file that does the following:

        • change the implementation of createPartyRole from entity-auto to simple the way it was before.
        • revert the change of the service implementation in OFBIZ-6703 as we do not need it anymore.
        Show
        taher Taher Alkhateeb added a comment - Hi Jacques, I've added a patch file that does the following: change the implementation of createPartyRole from entity-auto to simple the way it was before. revert the change of the service implementation in OFBIZ-6703 as we do not need it anymore.
        Hide
        gil portenseigne Gil Portenseigne added a comment -

        Hi Taher,

        I just add a comment here OFBIZ-6703 that will present another (better?) fix proposal.

        Show
        gil portenseigne Gil Portenseigne added a comment - Hi Taher, I just add a comment here OFBIZ-6703 that will present another (better?) fix proposal.
        Hide
        gil portenseigne Gil Portenseigne added a comment - - edited

        Fixed in
        Trunk r1714976
        14.12 r1714977

        I did review all createPartyRole service call in *.*, and replace some of the occurence with ensurePartyRole, one in SubscriptionServices.java that could cause issue, and others to ease the readability.

        The ones i left were mainly used in party creation...

        Show
        gil portenseigne Gil Portenseigne added a comment - - edited Fixed in Trunk r1714976 14.12 r1714977 I did review all createPartyRole service call in *.*, and replace some of the occurence with ensurePartyRole, one in SubscriptionServices.java that could cause issue, and others to ease the readability. The ones i left were mainly used in party creation...
        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        Thanks Gil, I must say we all missed to introduce ensurePartyRole in the picture, which was the right answer!

        Show
        jacques.le.roux Jacques Le Roux added a comment - Thanks Gil, I must say we all missed to introduce ensurePartyRole in the picture, which was the right answer!
        Hide
        soledad Nicolas Malin added a comment -

        Greats ! thanks Taher and Jacques to remind this issues and Gil for the final check. I missed time to close it myself

        Show
        soledad Nicolas Malin added a comment - Greats ! thanks Taher and Jacques to remind this issues and Gil for the final check. I missed time to close it myself

          People

          • Assignee:
            jacques.le.roux Jacques Le Roux
            Reporter:
            soledad Nicolas Malin
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development