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

Change createPartyRelationshipAndRole to use ensurePartyRole

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: Trunk
    • Fix Version/s: 14.12.01, 16.11.01
    • Component/s: party
    • Labels:
      None

      Description

      createPartyRelationshipAndRole in party/servicedef/services.xml invokes createPartyRole and then createPartyRelationship. It would be better if createPartyRole was changed to ensurePartyRole so that createPartyRelationshipAndRole can be called without the risk of a duplicate key error in PartyRole

        Issue Links

          Activity

          Hide
          gareth.carter Gareth Carter added a comment -

          createPartyRole has permission checking whereas ensurePartyRole does not, changing this will slightly change the behaviour. I am not entirely sure if this would be correct. Obviously creating a party relationship requires the party roles to exist in the database because of referential integrity. Why do we need permissions around creating roles if roles are required to exist in PartyRole for other entities, is the PartyRole entity only for referential integrity or used in other ways aswell?

          Show
          gareth.carter Gareth Carter added a comment - createPartyRole has permission checking whereas ensurePartyRole does not, changing this will slightly change the behaviour. I am not entirely sure if this would be correct. Obviously creating a party relationship requires the party roles to exist in the database because of referential integrity. Why do we need permissions around creating roles if roles are required to exist in PartyRole for other entities, is the PartyRole entity only for referential integrity or used in other ways aswell?
          Hide
          jacques.le.roux Jacques Le Roux added a comment - - edited

          HI Gareth, there is currently a discussion about createPartyRole vs ensurePartyRole in the dev ML after OFBIZ-5853. In my opininon we should replace the createPartyRole implementation by the ensurePartyRole one

          Show
          jacques.le.roux Jacques Le Roux added a comment - - edited HI Gareth, there is currently a discussion about createPartyRole vs ensurePartyRole in the dev ML after OFBIZ-5853 . In my opininon we should replace the createPartyRole implementation by the ensurePartyRole one
          Hide
          gareth.carter Gareth Carter added a comment -

          Thanks Jacques, I am in agreement with you. For me ensurePartyRole is a better descriptive service name than keeping createPartyRole and changing the service description as suggested by the discussion.

          Not sure what you want to do with this issue as it's pretty much a duplicate of 5853

          Show
          gareth.carter Gareth Carter added a comment - Thanks Jacques, I am in agreement with you. For me ensurePartyRole is a better descriptive service name than keeping createPartyRole and changing the service description as suggested by the discussion. Not sure what you want to do with this issue as it's pretty much a duplicate of 5853
          Hide
          soledad Nicolas Malin added a comment -

          I check the service's definition and it's strange because it use a group to call createPartyRole and createPartyRelationship. But the createPartyRole search a partyId and roleTypeId in the context and createPartyRelationship partyIdFrom, roleTypeIdFrom and partyIdTo, roleTypeIdTo.

          To improvement sur buisness framework I propose :

          • Convert createPartyRole to ensurePartyRole
          • Add description ont the service because it's mistaking to have a service with different field
          • Add a new service createPartyRelationshipAndRoles that call
            • ensurePartyRoleFrom (new service)
            • ensurePartyRoleThru (new service)
            • createPartyRelationship

          what do you means for that ?

          Show
          soledad Nicolas Malin added a comment - I check the service's definition and it's strange because it use a group to call createPartyRole and createPartyRelationship. But the createPartyRole search a partyId and roleTypeId in the context and createPartyRelationship partyIdFrom, roleTypeIdFrom and partyIdTo, roleTypeIdTo. To improvement sur buisness framework I propose : Convert createPartyRole to ensurePartyRole Add description ont the service because it's mistaking to have a service with different field Add a new service createPartyRelationshipAndRoles that call ensurePartyRoleFrom (new service) ensurePartyRoleThru (new service) createPartyRelationship what do you means for that ?
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          Hi Nicolas,

          By "Convert createPartyRole to ensurePartyRole" I guess you mean to replace createPartyRole implementation by ensurePartyRole implementation as I suggested?

          For your other points I agree we need ensurePartyRoleFrom and ensurePartyRoleThru. But I don't see a reason to keep the current implementation of createPartyRelationshipAndRoles. So I guess by "Add a new service createPartyRelationshipAndRoles" you mean to replace the current implementation of createPartyRelationshipAndRoles, right?

          BTW while at it we should replace the ensurePartyRole definition description by its implementation description, thanks!

          Show
          jacques.le.roux Jacques Le Roux added a comment - Hi Nicolas, By "Convert createPartyRole to ensurePartyRole" I guess you mean to replace createPartyRole implementation by ensurePartyRole implementation as I suggested? For your other points I agree we need ensurePartyRoleFrom and ensurePartyRoleThru. But I don't see a reason to keep the current implementation of createPartyRelationshipAndRoles. So I guess by "Add a new service createPartyRelationshipAndRoles" you mean to replace the current implementation of createPartyRelationshipAndRoles, right? BTW while at it we should replace the ensurePartyRole definition description by its implementation description, thanks!
          Hide
          gareth.carter Gareth Carter added a comment -

          Why doesn't createPartyRelationship ensure the PartyRole records exists? Surely that would be easier? roleTypeIdTo and roleTypeIdFrom are part of the PK for PartyRelationship so to me they are a requirement and cannot be null. Or alternatively you could remove the constraint to PartyRole and just have to RoleType entity?

          Show
          gareth.carter Gareth Carter added a comment - Why doesn't createPartyRelationship ensure the PartyRole records exists? Surely that would be easier? roleTypeIdTo and roleTypeIdFrom are part of the PK for PartyRelationship so to me they are a requirement and cannot be null. Or alternatively you could remove the constraint to PartyRole and just have to RoleType entity?
          Hide
          soledad Nicolas Malin added a comment -

          Ok I commit a the service correction and new service ensurePartyRoleForm et To on rev 1649965.

          A tested the resolution on sfa screen, and it's solve the problem.

          Thanks Gareth for your issuing

          Show
          soledad Nicolas Malin added a comment - Ok I commit a the service correction and new service ensurePartyRoleForm et To on rev 1649965. A tested the resolution on sfa screen, and it's solve the problem. Thanks Gareth for your issuing
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          Thanks Nicolas!

          Show
          jacques.le.roux Jacques Le Roux added a comment - Thanks Nicolas!
          Hide
          soledad Nicolas Malin added a comment -

          I fixed it also on branch 14.12 under revision 1650007

          Show
          soledad Nicolas Malin added a comment - I fixed it also on branch 14.12 under revision 1650007
          Hide
          jacques.le.roux Jacques Le Roux added a comment -

          Other releases too complicated, or?

          Show
          jacques.le.roux Jacques Le Roux added a comment - Other releases too complicated, or?

            People

            • Assignee:
              soledad Nicolas Malin
              Reporter:
              gareth.carter Gareth Carter
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development