Details

    • Type: Improvement Improvement
    • Status: Patch Available
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: Trunk
    • Fix Version/s: None
    • Component/s: party, product
    • Labels:
      None
    • Sprint:
      Bug Crush Event - 21/2/2015

      Description

      Despite how much I typed; this is really a very small patch.

      This patch adds a new entity "SupplierRel" which is a sub-class of "PartyRelationship" (as well as a view-entity for convenience). It provides a new field "accountNumber" that can be used to store the long-term account number assigned to the relationship between the Company and its Supplier. The life of this account number is longer than any agreement between the two, so it has been put on this informal relationship. Moreover, it is possible to have an informal relationship between a company and a supplier with out an explicit binding agreement – this was discussed most recently in this thread:

      http://ofbiz.135035.n4.nabble.com/Storing-supplier-provided-account-number-td2076162.html#a2076162

      ALSO – this patch fixes two problems that I encountered when attempting to create a party relationship.

      a) It did not look right to have an empty dropdown for status – I created the standard "Created" status under the PARTY_REL_STATUS type so that we show the only applicable status. There does not appear to be any specific logic looking for party relationships with a blank status, so creating ones with this status should not cause any issues.

      b) When creating the PartyRelationship the response in the controller was of type "view-last" which was a problem because the last controller request was typically the ajax one to "FindPartyName" which was used as part of the party lookup field in that form. The net result, was that on success it would render the PartyName instead of replaying the EditPartyRelationships. Changed form "view-last" to "view" to resolve this issue.

        Activity

        Hide
        Mridul Pathak added a comment -

        I like the second approach better.

        Show
        Mridul Pathak added a comment - I like the second approach better.
        Hide
        Jacques Le Roux added a comment -

        Right Pierre, done.

        Show
        Jacques Le Roux added a comment - Right Pierre, done.
        Hide
        Pierre Smits added a comment -

        Maybe you should consider changing the subject of this issue even more to make it more applicable to what we are talking about: Party Relationship Sub-Classes.

        Because the potential entity model adjustment will not be just for suppliers, right?

        Show
        Pierre Smits added a comment - Maybe you should consider changing the subject of this issue even more to make it more applicable to what we are talking about: Party Relationship Sub-Classes. Because the potential entity model adjustment will not be just for suppliers, right?
        Hide
        Jacques Le Roux added a comment -

        I prefer to let open other possiblities

        Show
        Jacques Le Roux added a comment - I prefer to let open other possiblities
        Hide
        Pierre Smits added a comment -

        Should

        <PartyRelationshipType description="" hasTable="Y" parentTypeId="" partyRelationshipName="Supplier" partyRelationshipTypeId="SUPPLIER_REL" roleTypeIdValidFrom="" roleTypeIdValidTo=""/>
        

        not have the following roleTypes set:

        • roleTypeIdValidFrom="INTERNAL_ORGANIZATIO", and
        • roleTypeIdValidTo="SUPPLIER" ?
        Show
        Pierre Smits added a comment - Should <PartyRelationshipType description= "" hasTable=" Y " parentTypeId=" " partyRelationshipName=" Supplier " partyRelationshipTypeId=" SUPPLIER_REL " roleTypeIdValidFrom=" " roleTypeIdValidTo=" "/> not have the following roleTypes set: roleTypeIdValidFrom="INTERNAL_ORGANIZATIO", and roleTypeIdValidTo="SUPPLIER" ?
        Hide
        Jacques Le Roux added a comment -

        Yes see my comment below

        Show
        Jacques Le Roux added a comment - Yes see my comment below
        Hide
        Jacques Le Roux added a comment -

        I committed the other part of the patch Bob suggested
        <<It did not look right to have an empty dropdown for status – I created the standard "Created" status under the PARTY_REL_STATUS type so that we show the only applicable status. There does not appear to be any specific logic looking for party relationships with a blank status, so creating ones with this status should not cause any issues.>>
        at revision: 1651631

        I added an expired status and a valid transition. Since this is not a bug fix but an improvement I did not backport

        Show
        Jacques Le Roux added a comment - I committed the other part of the patch Bob suggested <<It did not look right to have an empty dropdown for status – I created the standard "Created" status under the PARTY_REL_STATUS type so that we show the only applicable status. There does not appear to be any specific logic looking for party relationships with a blank status, so creating ones with this status should not cause any issues.>> at revision: 1651631 I added an expired status and a valid transition. Since this is not a bug fix but an improvement I did not backport
        Hide
        Pierre Smits added a comment -

        So this is actually an improvement issue?

        Show
        Pierre Smits added a comment - So this is actually an improvement issue?
        Hide
        Jacques Le Roux added a comment - - edited

        One of the bugs Bob suggested to fix has been already fixed by OFBIZ-3658

        Show
        Jacques Le Roux added a comment - - edited One of the bugs Bob suggested to fix has been already fixed by OFBIZ-3658
        Hide
        Jacques Le Roux added a comment - - edited

        To make things more clear here is a summary for the community:
        We found 2 possibilities

        1. Add a new sub-class of PartyRelationship, like PartyRelationId as I suggested based on what Bob suggested. We would then need to put the relationId I suggested in the PK.
          • Pros: it's simple and does not entail to make a relation with PartyRelationship and to add specific types for the relationId which can be many. More than PartyIdentificationType I guess.
          • Cons: it's maybe too simple if we want to detail the specific types of the relations
        2. Add another partyRelIdentificationId field in the PK of a new PartyRelIdentification entity as Nicolas suggested (based on Adrian's suggestion to use PartyIdentification). We should also create the new PartyRelIdentificationType for dedicated types
          • Pros: the opposite ot the other solution
          • Cons: the opposite ot the other solution

        Now I think it's the community to decide what we want

        Show
        Jacques Le Roux added a comment - - edited To make things more clear here is a summary for the community: We found 2 possibilities Add a new sub-class of PartyRelationship, like PartyRelationId as I suggested based on what Bob suggested. We would then need to put the relationId I suggested in the PK. Pros: it's simple and does not entail to make a relation with PartyRelationship and to add specific types for the relationId which can be many. More than PartyIdentificationType I guess. Cons: it's maybe too simple if we want to detail the specific types of the relations Add another partyRelIdentificationId field in the PK of a new PartyRelIdentification entity as Nicolas suggested (based on Adrian's suggestion to use PartyIdentification). We should also create the new PartyRelIdentificationType for dedicated types Pros: the opposite ot the other solution Cons: the opposite ot the other solution Now I think it's the community to decide what we want
        Hide
        Jacques Le Roux added a comment -

        BTW Nicolas, you did not answer to my last question

        Show
        Jacques Le Roux added a comment - BTW Nicolas, you did not answer to my last question
        Hide
        Jacques Le Roux added a comment -

        Yes we need to put it in the PK if we want to fulfill Ron's requirement of multi values. See my last answer to you above for a detailed reply

        Show
        Jacques Le Roux added a comment - Yes we need to put it in the PK if we want to fulfill Ron's requirement of multi values. See my last answer to you above for a detailed reply
        Hide
        Jacques Le Roux added a comment -

        Nicolas you said

        I don't see the improvement betwee this and adding directly the new field on PartyRelationship entity.

        Actually it's just a matter of organisation. The idea is to not put too much fields in PartyRelationship and for that create a specific entity. Like we have the Employment entity for instance.

        But the problem is now if we want to have many account numbers between 2 parties, as mentionned Ron.

        So there are still 2 possibilities

        1. Add a new sub-class of PartyRelationship, like PartyRelationId as I suggested based on what Bob suggested. We would then need to put the relationId I suggested in the PK.
          • Pros: it's simple and does not entail to make a relation with PartyRelationship and to add specific types for the relationId which can be many. More than PartyIdentificationType I guess.
          • Cons: it's maybe too simple if we want to detail the specific types of the relationId
        2. Add another partyRelIdentificationId field in the PK of a new PartyRelIdentification entity as you suggestted. We should alson create a new PartyRelIdentificationType for dedicated types
          • Pros: the opposite ot the other solution
          • Cons: the opposite ot the other solution

        Now I think it's the community to decide what we want

        Show
        Jacques Le Roux added a comment - Nicolas you said I don't see the improvement betwee this and adding directly the new field on PartyRelationship entity. Actually it's just a matter of organisation. The idea is to not put too much fields in PartyRelationship and for that create a specific entity. Like we have the Employment entity for instance. But the problem is now if we want to have many account numbers between 2 parties, as mentionned Ron. So there are still 2 possibilities Add a new sub-class of PartyRelationship, like PartyRelationId as I suggested based on what Bob suggested. We would then need to put the relationId I suggested in the PK. Pros: it's simple and does not entail to make a relation with PartyRelationship and to add specific types for the relationId which can be many. More than PartyIdentificationType I guess. Cons: it's maybe too simple if we want to detail the specific types of the relationId Add another partyRelIdentificationId field in the PK of a new PartyRelIdentification entity as you suggestted. We should alson create a new PartyRelIdentificationType for dedicated types Pros: the opposite ot the other solution Cons: the opposite ot the other solution Now I think it's the community to decide what we want
        Hide
        Nicolas Malin added a comment -

        relationId is a pk ? or link to an other entity pk ?

        Because if not, in my brain (maybe wrongly) all fieldId are pk on OFBiz entity. The reason that GoodIdentification and PartyIdentification use idValue instead of valueId. Some old entities contains some fieldId like not a pk managed by OFBiz and it's disturbing

        Show
        Nicolas Malin added a comment - relationId is a pk ? or link to an other entity pk ? Because if not, in my brain (maybe wrongly) all fieldId are pk on OFBiz entity. The reason that GoodIdentification and PartyIdentification use idValue instead of valueId. Some old entities contains some fieldId like not a pk managed by OFBiz and it's disturbing
        Hide
        Nicolas Malin added a comment -

        Hmmm sure it's a strange case

        I think isn't a better way to manage your several accounts. The product component work well.

        The thread origin is adding adding an accountNumber who is an identification string. But accountNumber is so restrictive and we can set only one value on a relation. I don't see the improvement betwee this and adding directly the new field on PartyRelationship entity.

        So the main ask for me is what we want set as value on this new entity. My proposition run only on identification information.
        I can imagine an example :

        • DemoCustomer CUSTOMER DemoSupplier SUPPLIER 2015-01-01 00:00:00 SUPPLIER_REL ACCOUNT_NUMBER 12343
        • DemoCustomer CUSTOMER DemoSupplier SUPPLIER 2015-01-01 00:00:00 SUPPLIER_REL REFERENCE_SYS E4564R
        • DemoSupplier SUPPLIER DemoCustomer CUSTOMER 2015-01-01 00:00:00 CUSTOMER_REL REFERENCE_SYS 10023
          We can use these information by an automate to exchange with the customer erp and supplier erp directly with their own partyId's system

        If we want set more than one ACCOUNT_NUMBER identification, it's possible to add another pk partyRelIdentificationId to have :

        • 10010 DemoCustomer CUSTOMER DemoSupplier SUPPLIER 2015-01-01 00:00:00 SUPPLIER_REL ACCOUNT_NUMBER 12343
        • 10011 DemoCustomer CUSTOMER DemoSupplier SUPPLIER 2015-01-01 00:00:00 SUPPLIER_REL ACCOUNT_NUMBER 123265
          But I think isn't easier to use an automatic resolution to use what account to sharing when.

        Pierre, I understand your proposition to use more generalization but set an enumValue or enumId it's to me define a range value, it's will better to use the EntityAttr pattern to define the generalization (so PartyRelationshipAttr)

        Show
        Nicolas Malin added a comment - Hmmm sure it's a strange case I think isn't a better way to manage your several accounts. The product component work well. The thread origin is adding adding an accountNumber who is an identification string. But accountNumber is so restrictive and we can set only one value on a relation. I don't see the improvement betwee this and adding directly the new field on PartyRelationship entity. So the main ask for me is what we want set as value on this new entity. My proposition run only on identification information. I can imagine an example : DemoCustomer CUSTOMER DemoSupplier SUPPLIER 2015-01-01 00:00:00 SUPPLIER_REL ACCOUNT_NUMBER 12343 DemoCustomer CUSTOMER DemoSupplier SUPPLIER 2015-01-01 00:00:00 SUPPLIER_REL REFERENCE_SYS E4564R DemoSupplier SUPPLIER DemoCustomer CUSTOMER 2015-01-01 00:00:00 CUSTOMER_REL REFERENCE_SYS 10023 We can use these information by an automate to exchange with the customer erp and supplier erp directly with their own partyId's system If we want set more than one ACCOUNT_NUMBER identification, it's possible to add another pk partyRelIdentificationId to have : 10010 DemoCustomer CUSTOMER DemoSupplier SUPPLIER 2015-01-01 00:00:00 SUPPLIER_REL ACCOUNT_NUMBER 12343 10011 DemoCustomer CUSTOMER DemoSupplier SUPPLIER 2015-01-01 00:00:00 SUPPLIER_REL ACCOUNT_NUMBER 123265 But I think isn't easier to use an automatic resolution to use what account to sharing when. Pierre, I understand your proposition to use more generalization but set an enumValue or enumId it's to me define a range value, it's will better to use the EntityAttr pattern to define the generalization (so PartyRelationshipAttr)
        Hide
        Jacques Le Roux added a comment -

        OK, with Ron's comment about Nicolas's propostion, I think we should rather follow Bob's way with the adaptation we spoke already about. Now we need to know if we should rather use simple Ids (as I suggested with relationId) or if we should qualify them as suggested Pierre by replacing <field name="accountNumber" with <field name="enumValue" and adding <field name="enumId". It seems to me that there are a lot of possiilities, of course everyone could create the ones she needs, but do we really want that?

        Show
        Jacques Le Roux added a comment - OK, with Ron's comment about Nicolas's propostion, I think we should rather follow Bob's way with the adaptation we spoke already about. Now we need to know if we should rather use simple Ids (as I suggested with relationId) or if we should qualify them as suggested Pierre by replacing <field name="accountNumber" with <field name="enumValue" and adding <field name="enumId". It seems to me that there are a lot of possiilities, of course everyone could create the ones she needs, but do we really want that?
        Hide
        Jacques Le Roux added a comment - - edited

        Well spotted Ron, with the PartyIdentification PK being partyId+partyIdentificationTypeId this is indeed a problem!

        Show
        Jacques Le Roux added a comment - - edited Well spotted Ron, with the PartyIdentification PK being partyId+partyIdentificationTypeId this is indeed a problem!
        Hide
        Jacques Le Roux added a comment -

        Yes of course (my preferred god) but we need to think, and especially discuss, more about Nicolas's proposition before... No hurry here...

        Show
        Jacques Le Roux added a comment - Yes of course (my preferred god) but we need to think, and especially discuss, more about Nicolas's proposition before... No hurry here...
        Hide
        Jacques Le Roux added a comment - - edited

        I have to think about Adrian's and your proposition Nicolas. It makes also sense, it's just a bit more involved but there is indeed almost all what we need, but the new PartyRelIdentificationTypes which could be created as will.

        One question though, how to you envision to relate PartyRelationship and PartyRelIdentification? By adding a partyRelId field I guess? I just fear this can be confusing (too convoluted) for users, maybe not if we provide demo examples?

        Show
        Jacques Le Roux added a comment - - edited I have to think about Adrian's and your proposition Nicolas. It makes also sense, it's just a bit more involved but there is indeed almost all what we need, but the new PartyRelIdentificationTypes which could be created as will. One question though, how to you envision to relate PartyRelationship and PartyRelIdentification? By adding a partyRelId field I guess? I just fear this can be confusing (too convoluted) for users, maybe not if we provide demo examples?
        Hide
        Pierre Smits added a comment -

        Well, you can always claim YAGNI

        Show
        Pierre Smits added a comment - Well, you can always claim YAGNI
        Hide
        Jacques Le Roux added a comment -

        Neat point Pierre, in Bob's proposition there is already SupplierRel.fromDate. I guess Bob just forgot the thruDate.

        So we would add a thruDate to the new PartyRelAccount I propose. As usual, it would not be part of the PK.

        I wonder if we would not get too far by replacing <field name="accountNumber" with <field name="enumValue" and adding <field name="enumId".
        What we need here is simply an id so far. I'd simply replace
        <field name="accountNumber" type="name"/>
        by
        <field name="relationId" type="id-ne"/>

        Actually I even propose to rename the entity PartyRelationId rather than PartyRelAccount.

        Show
        Jacques Le Roux added a comment - Neat point Pierre, in Bob's proposition there is already SupplierRel.fromDate. I guess Bob just forgot the thruDate. So we would add a thruDate to the new PartyRelAccount I propose. As usual, it would not be part of the PK. I wonder if we would not get too far by replacing <field name="accountNumber" with <field name="enumValue" and adding <field name="enumId". What we need here is simply an id so far. I'd simply replace <field name="accountNumber" type="name"/> by <field name="relationId" type="id-ne"/> Actually I even propose to rename the entity PartyRelationId rather than PartyRelAccount.
        Hide
        Pierre Smits added a comment -

        We have to take into considerations that these account numbers, etc also have a lifespan.

        Show
        Pierre Smits added a comment - We have to take into considerations that these account numbers, etc also have a lifespan.
        Hide
        Jacques Le Roux added a comment -

        quoting Ron on dev ML

        Will this handle the situation where the company has a number of accounts or contracts with a supplier?
        For example a condo association might have several account numbers with the electric utility if there are several buildings or meters.
        Same for telecom if there are a number of different services purchased from the telecom server but billed with different account numbers. (Bell Canada does this)

        Show
        Jacques Le Roux added a comment - quoting Ron on dev ML Will this handle the situation where the company has a number of accounts or contracts with a supplier? For example a condo association might have several account numbers with the electric utility if there are several buildings or meters. Same for telecom if there are a number of different services purchased from the telecom server but billed with different account numbers. (Bell Canada does this)
        Hide
        Pierre Smits added a comment -

        I agree, but the suggestion by Jacques seems to be a good compromise.

        Show
        Pierre Smits added a comment - I agree, but the suggestion by Jacques seems to be a good compromise.
        Hide
        Nicolas Malin added a comment -

        I'm not fan that entites whit business oriented name like Supplier*, so the Adrian's proposition is just better for me .

        If the cover is associate different value like an account on a relation, with do not use the same pattern like PartyIdentification ?

        PartyRelationship * - * PartyRelIdentification * - PartyIdentificationType

        or with dedicate Type :

        PartyRelationship * - * PartyRelIdentification * - PartyRelIdentificationType

        Show
        Nicolas Malin added a comment - I'm not fan that entites whit business oriented name like Supplier*, so the Adrian's proposition is just better for me . If the cover is associate different value like an account on a relation, with do not use the same pattern like PartyIdentification ? PartyRelationship * - * PartyRelIdentification * - PartyIdentificationType or with dedicate Type : PartyRelationship * - * PartyRelIdentification * - PartyRelIdentificationType
        Hide
        Pierre Smits added a comment -

        If you would replace <field name="accountNumber" with <field name="enumValue" with the appropriate type, you would have some more generalization, especially when adding also <field name="enumId" ....

        But I also guess that both would need to be available for the fromParty as the toParty

        Show
        Pierre Smits added a comment - If you would replace <field name="accountNumber" with <field name="enumValue" with the appropriate type, you would have some more generalization, especially when adding also <field name="enumId" .... But I also guess that both would need to be available for the fromParty as the toParty
        Hide
        Jacques Le Roux added a comment -

        Of course more fields could be added later to these new entities... if needed (YAGNI principle)

        Show
        Jacques Le Roux added a comment - Of course more fields could be added later to these new entities... if needed (YAGNI principle)
        Hide
        Jacques Le Roux added a comment - - edited

        I had a new look and was interested by Adrian Crum's comment:

        We have similar needs here with parties in other relationships. Our
        dealers have dealer numbers and license numbers, our suppliers give us
        numbers (the same as the previous thread), and the homeowners have a
        customer number and other bits of information we have to maintain for them.

        Taking Adrian's comment into account I think the proposed data model is good but it needs to be generaliseds since experience shows that not only suppliers are concerned but other party types as well
        The names of the new entities should be changed from

        • SupplierRel to PartyRelAccount
        • SupplierRelAndPartyRelationship to PartyRelAccountAndPartyRelationship
          and those entities moved from
          package-name="org.ofbiz.product.supplier"
          to package-name="org.ofbiz.party.party"
          Of course the description should be also updated

        The rest sounds good to me. If nobody see a problem with that I will commit a slightly modified patch soon (yes it still applies)

        Show
        Jacques Le Roux added a comment - - edited I had a new look and was interested by Adrian Crum 's comment: We have similar needs here with parties in other relationships. Our dealers have dealer numbers and license numbers, our suppliers give us numbers (the same as the previous thread), and the homeowners have a customer number and other bits of information we have to maintain for them. Taking Adrian's comment into account I think the proposed data model is good but it needs to be generaliseds since experience shows that not only suppliers are concerned but other party types as well The names of the new entities should be changed from SupplierRel to PartyRelAccount SupplierRelAndPartyRelationship to PartyRelAccountAndPartyRelationship and those entities moved from package-name="org.ofbiz.product.supplier" to package-name="org.ofbiz.party.party" Of course the description should be also updated The rest sounds good to me. If nobody see a problem with that I will commit a slightly modified patch soon (yes it still applies)
        Hide
        Jacques Le Roux added a comment -

        This looks good to me, though I did not check the PartyTypeData issue

        Show
        Jacques Le Roux added a comment - This looks good to me, though I did not check the PartyTypeData issue
        Hide
        Bob Morley added a comment -

        Quick note: generally I would create individual patches for this; but because they were so small I included them in a single patch file. Feel free to split up the "two minor fixes" from the enhancement if so desired.

        Show
        Bob Morley added a comment - Quick note: generally I would create individual patches for this; but because they were so small I included them in a single patch file. Feel free to split up the "two minor fixes" from the enhancement if so desired.

          People

          • Assignee:
            Jacques Le Roux
            Reporter:
            Bob Morley
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:

              Development

                Agile