OFBiz
  1. OFBiz
  2. OFBIZ-547

Duplicate Id error on Create New FixedAsset

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: Trunk
    • Component/s: accounting
    • Labels:
      None

      Description

      While creating a New FixedAsset, if user enters FixedAssetId that already exists in system, Unfriendly message is returned and existing fixedAsset is opened in Edit mode.
      1) The error message should be easier to understand by user.
      2) Existing fixedAsset should not be opened in edit mode, this confuses user with Idea that fixedAsset was created.

        Activity

        Hide
        Anil K Patel added a comment -

        This patch gracefully handles submit if the user entered fixedAssetId that already existed in system. I'll like to get some Idea on How we can make the Form to now show existing FixedAsset in Edit mode, as a result of this error.

        Show
        Anil K Patel added a comment - This patch gracefully handles submit if the user entered fixedAssetId that already existed in system. I'll like to get some Idea on How we can make the Form to now show existing FixedAsset in Edit mode, as a result of this error.
        Hide
        Anil K Patel added a comment -

        Sorry, a mistake in above comments it should read as following

        This patch gracefully handles submit if the user entered fixedAssetId that already existed in system. I'll like to get some Idea on How we can make the Form to NOT show existing FixedAsset in Edit mode, as a result of this error.

        Show
        Anil K Patel added a comment - Sorry, a mistake in above comments it should read as following This patch gracefully handles submit if the user entered fixedAssetId that already existed in system. I'll like to get some Idea on How we can make the Form to NOT show existing FixedAsset in Edit mode, as a result of this error.
        Hide
        Anil K Patel added a comment -

        The attached patch implements handles duplicate primary key error. Implements most common pattern for solving such problem in Ofbiz. This very small patch should not take lot of time for a commiter to review.

        Show
        Anil K Patel added a comment - The attached patch implements handles duplicate primary key error. Implements most common pattern for solving such problem in Ofbiz. This very small patch should not take lot of time for a commiter to review.
        Hide
        Chris Howe added a comment -

        Thanks for looking at this Anil. There is an instance that this solution doesn't handle. Since the sequence-id-to-env simply retrieves a value based on a field in SequenceValueItem, and the database is unaware of it's importance, it is possible for the SequenceValueItem to be out of sync with the next non duplicated primary key value. This may occur in a deployment that does entity syncs or imports.

        Is this pattern already used, or are you suggesting it?

        Additionally, since you're suppressing the error from occurring, you may want to put something to the log file perhaps showing the entire FixedAsset record that was retrieved (but certainly not to the screen as the user may not have privilege to view that record).

        Show
        Chris Howe added a comment - Thanks for looking at this Anil. There is an instance that this solution doesn't handle. Since the sequence-id-to-env simply retrieves a value based on a field in SequenceValueItem, and the database is unaware of it's importance, it is possible for the SequenceValueItem to be out of sync with the next non duplicated primary key value. This may occur in a deployment that does entity syncs or imports. Is this pattern already used, or are you suggesting it? Additionally, since you're suppressing the error from occurring, you may want to put something to the log file perhaps showing the entire FixedAsset record that was retrieved (but certainly not to the screen as the user may not have privilege to view that record).
        Hide
        Anil K Patel added a comment -

        In create FixedAsset form, user can enter FixedAssetId. Just because we allow user to enter FixedAssetId, there are changes that they may enter a value that already exists in Database. This patch is to handle such situation.
        While I am aware that user should not enter value in fixedAssetId field, The fact is we have a text box for this field. So we cannot deny this situation.

        Show
        Anil K Patel added a comment - In create FixedAsset form, user can enter FixedAssetId. Just because we allow user to enter FixedAssetId, there are changes that they may enter a value that already exists in Database. This patch is to handle such situation. While I am aware that user should not enter value in fixedAssetId field, The fact is we have a text box for this field. So we cannot deny this situation.
        Hide
        Chris Howe added a comment -

        I'm not referring to the part that handles the form. That part looks fine.

        The part that currently exists in the code (modified with your patch):

        <if-empty field-name="parameters.fixedAssetId">
        <sequenced-id-to-env sequence-name="FixedAsset" env-name="newEntity.fixedAssetId"/>
        <else>

        Can return a duplicate Id. It can before the patch and it can after the patch. So the returned value should be checked as well.

        Since this issue is handling error messages for duplicate Ids we should fix it here and also send a little bit of information to the log.

        Show
        Chris Howe added a comment - I'm not referring to the part that handles the form. That part looks fine. The part that currently exists in the code (modified with your patch): <if-empty field-name="parameters.fixedAssetId"> <sequenced-id-to-env sequence-name="FixedAsset" env-name="newEntity.fixedAssetId"/> <else> Can return a duplicate Id. It can before the patch and it can after the patch. So the returned value should be checked as well. Since this issue is handling error messages for duplicate Ids we should fix it here and also send a little bit of information to the log.
        Hide
        Anil K Patel added a comment -

        Does that mean
        we should clean cache of PrimaryKey bank before asking for new key
        or
        should we write a loop that will keep trying next available usable primary key value

        Show
        Anil K Patel added a comment - Does that mean we should clean cache of PrimaryKey bank before asking for new key or should we write a loop that will keep trying next available usable primary key value
        Hide
        Chris Howe added a comment -

        cleaning cache wouldn't solve the problem as even the next 10 may be duplicates as well. Looping would solve the problem. However, this may be better addressed in the method underlying sequenced-id-to-env in the minilang java files. And looping until a good one is found may be performance intensive. If we could get some feedback from some others and they agree on solving it through the minilang java, that would be a new issue.

        Show
        Chris Howe added a comment - cleaning cache wouldn't solve the problem as even the next 10 may be duplicates as well. Looping would solve the problem. However, this may be better addressed in the method underlying sequenced-id-to-env in the minilang java files. And looping until a good one is found may be performance intensive. If we could get some feedback from some others and they agree on solving it through the minilang java, that would be a new issue.
        Hide
        Anil K Patel added a comment -

        Chris,
        From what I understand, this is problem with Ofbiz primary key generation system in general. If you are suggesting this should be a Issue in itself, Is it ok if this patch be applied. Because this patch solves the problem that we face when User enters value in Id field.

        what do you suggest?

        Show
        Anil K Patel added a comment - Chris, From what I understand, this is problem with Ofbiz primary key generation system in general. If you are suggesting this should be a Issue in itself, Is it ok if this patch be applied. Because this patch solves the problem that we face when User enters value in Id field. what do you suggest?
        Hide
        Chris Howe added a comment -

        I would vote for it as is if it added
        <log level="info" message="FixedAssetId already exists: [${[${parameters.fixedAssetId}}]". Current record: $

        {fixedAsset}

        />

        or something similar but internationalized.

        Show
        Chris Howe added a comment - I would vote for it as is if it added <log level="info" message="FixedAssetId already exists: [${ [${parameters.fixedAssetId}}] ". Current record: $ {fixedAsset} /> or something similar but internationalized.
        Hide
        Anil K Patel added a comment -

        This is not a problem I can do this.

        Show
        Anil K Patel added a comment - This is not a problem I can do this.
        Hide
        Anil K Patel added a comment -

        Chris, Thanks for you help on this.

        Show
        Anil K Patel added a comment - Chris, Thanks for you help on this.
        Hide
        Anil K Patel added a comment -

        This patch extends existing patch. Also send error message to log file. Thanks to Chris for help in improving this patch.

        Show
        Anil K Patel added a comment - This patch extends existing patch. Also send error message to log file. Thanks to Chris for help in improving this patch.
        Hide
        Jacques Le Roux added a comment -

        I did not test the patch but it seems sound and I vote for.

        Show
        Jacques Le Roux added a comment - I did not test the patch but it seems sound and I vote for.
        Hide
        Anil K Patel added a comment -

        Can one of the commiters please take a look at this patch and apply it. Its been in wait for long, It will save me maintenance of patch on a asset maintenance application.
        Its need now.

        Regards
        Anil Patel

        Show
        Anil K Patel added a comment - Can one of the commiters please take a look at this patch and apply it. Its been in wait for long, It will save me maintenance of patch on a asset maintenance application. Its need now. Regards Anil Patel
        Hide
        Jacques Le Roux added a comment -

        Anil,

        It seems that commiters have others important tasks to do with the TLP migration. I agree to apply your patch soon with the old good CTR policy. If anybody see a problem there please chim in...

        Show
        Jacques Le Roux added a comment - Anil, It seems that commiters have others important tasks to do with the TLP migration. I agree to apply your patch soon with the old good CTR policy. If anybody see a problem there please chim in...
        Hide
        Jacopo Cappellato added a comment -

        +1

        Show
        Jacopo Cappellato added a comment - +1
        Hide
        Jacques Le Roux added a comment -

        Anil,

        Your patch is in OFBiz revision: 492411

        Thanks

        Show
        Jacques Le Roux added a comment - Anil, Your patch is in OFBiz revision: 492411 Thanks
        Hide
        Anil K Patel added a comment -

        Jacques,
        Thanks for your time and help.
        Regards

        Show
        Anil K Patel added a comment - Jacques, Thanks for your time and help. Regards
        Hide
        David E. Jones added a comment -

        Yes, I guess a commit is a good way to get someone, especially someone like me, to review it!

        Anyway, this all looks pretty safe, and more closely follow the pattern used with the createProduct service.

        BTW, in general the use of "entity-one" is far more effective and less error prone than the use of "find-by-primary-key". The main reason for this is that the field mappings are done with sub-elements, not with a Map that must be created and worried about.

        Show
        David E. Jones added a comment - Yes, I guess a commit is a good way to get someone, especially someone like me, to review it! Anyway, this all looks pretty safe, and more closely follow the pattern used with the createProduct service. BTW, in general the use of "entity-one" is far more effective and less error prone than the use of "find-by-primary-key". The main reason for this is that the field mappings are done with sub-elements, not with a Map that must be created and worried about.
        Hide
        Jacques Le Roux added a comment -

        Thanks for the comment David, this is this kind of little notes that helps to better handle MiniLang.

        Show
        Jacques Le Roux added a comment - Thanks for the comment David, this is this kind of little notes that helps to better handle MiniLang.

          People

          • Assignee:
            Jacques Le Roux
            Reporter:
            Anil K Patel
          • Votes:
            2 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development