Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

IDEMPIERE-5836 Adding support for stocked expense product #2039

Closed
wants to merge 1 commit into from
Closed

IDEMPIERE-5836 Adding support for stocked expense product #2039

wants to merge 1 commit into from

Conversation

dpansheriya
Copy link
Contributor

@dpansheriya dpansheriya commented Oct 2, 2023 •
edited by CarlosRuiz-globalqss

https://idempiere.atlassian.net/browse/IDEMPIERE-5836

Pull Request Checklist

  • My code follows the code guidelines of this project
  • My code follows the best practices of this project
  • I have performed a self-review of my own code
  • My code is easy to understand and review.
  • I have checked my code and corrected any misspellings
  • In hard-to-understand areas, I have commented my code.
  • My changes generate no new warnings

Tests

  • I have tested the direct scenario that my code is solving
  • I checked all collaterals that can be affected by my changes, and tested other potential affected scenarios
  • New and existing unit tests pass locally with my changes
  • I have added unit tests that prove my fix is effective or that my feature works

Documentation

  • I have made corresponding changes to the documentation as follows:
    • New feature (non-breaking change which adds functionality): I have created the New Feature page in the project wiki explaining the functionality and how to use it. If relevant, I have committed sample data to the core seed to have usable examples in GardenWorld.
    • Breaking change (fix or feature that would cause existing functionality to not work as expected): I have documented the change in a clear way that everyone in the community can understand the impact of the change.
    • Improvement (improves and existing functionality): This documentation is needed if the improvement changes the way the user interacts with the system or the outcome of a process/task changes. If it is just, for instance, a performance improvement, documentation might not be needed.
  • The changed/added documentation is in the project wiki (not privately-hosted pdf files or links pointing to a company website) and is complete and self-explanatory.

CarlosRuiz-globalqss
CarlosRuiz-globalqss requested changes Oct 17, 2023
View reviewed changes
org.adempiere.base/src/org/compiere/model/MProduct.java
@@ -595,7 +595,7 @@ public boolean isItem()
@Override
public boolean isStocked ()
{
return super.isStocked() && isItem();
return super.isStocked() && (isItem() || PRODUCTTYPE_ExpenseType.equals(getProductType()));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please change the javadoc accordingly

org.adempiere.base/src/org/compiere/model/MProduct.java
@@ -698,7 +698,7 @@ protected boolean beforeSave (boolean newRecord)

// Reset Stocked if not Item
//AZ Goodwill: Bug Fix isStocked always return false
if (!PRODUCTTYPE_Item.equals(getProductType()))
if (!PRODUCTTYPE_Item.equals(getProductType()) && !PRODUCTTYPE_ExpenseType.equals(getProductType()))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please change the comment too.

Also, please check line 666 of MProduct, there is a validation and storage managing code when changing from Item to not-Item, not sure about the effect, probably changing from Item to Expense doesn't matter now if the Stocked flag is maintained.

Line 918 (beforeDelete) has also a validation about storage

migration/iD11/oracle/202309291327_IDEMPIERE-5836.sql
SET SQLBLANKLINES ON
SET DEFINE OFF

-- I forgot to set the DICTIONARY_ID_COMMENTS System Configurator
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment on line 5 is wrong "I forgot to set the DICTIONARY_ID_COMMENTS System Configurator" better to delete it or make it correct

migration/iD11/oracle/202309291327_IDEMPIERE-5836.sql

-- I forgot to set the DICTIONARY_ID_COMMENTS System Configurator
-- Aug 8, 2023, 1:27:56 PM IST
UPDATE AD_Field SET DisplayLogic='@IsSummary@=''N'' & @ProductType@=I | @ProductType@=E', AD_Reference_Value_ID=NULL, AD_Val_Rule_ID=NULL, IsToolbarButton=NULL,Updated=TO_DATE('2023-08-08 13:27:56','YYYY-MM-DD HH24:MI:SS'),UpdatedBy=100 WHERE AD_Field_ID=1026
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic here is wrong, is showing the Stocked flag when the product is marked as Summary and Expense.

You need to add parenthesis on the OR condition, or reorder it to check the IsSummary at the end

migration/iD11/oracle/202309291327_IDEMPIERE-5836.sql
;

-- Oct 2, 2023, 3:38:33 PM IST
UPDATE AD_Column SET Callout='org.compiere.model.CalloutProduct.productType',Updated=TO_TIMESTAMP('2023-10-02 15:38:33','YYYY-MM-DD HH24:MI:SS'),UpdatedBy=100 WHERE AD_Column_ID=7795
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The CalloutProduct class is missing on the pull request

org.adempiere.base/src/org/compiere/acct/Doc_InOut.java
@@ -199,6 +199,11 @@ public ArrayList<Fact> createFacts (MAcctSchema as)
Map<String, BigDecimal> batchLotCostMap = null;
DocLine_InOut line = (DocLine_InOut) p_lines[i];
MProduct product = line.getProduct();
//If expense type stocked product, no impact on COGS as it not deducting inventory
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when doing a manual material receipt - is complaining with "No Costs for ..." - if the expense product is not to be posted, then the posting must not throw error

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We were creating cost. now creating new PR which has no cost created at all and so it will not throw any error.

org.adempiere.base/src/org/compiere/acct/Doc_Inventory.java
@@ -245,6 +245,11 @@ else if (MAcctSchema.COSTINGLEVEL_BatchLot.equals(costingLevel))
}
else
{
//if product type expense and stocked, then no needs to do posting
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Posting an Internal Use Inventory is throwing this error:
15:42:49.073===========> Doc_Inventory.post: [371]
java.lang.NullPointerException: Cannot invoke "org.compiere.model.MProduct.getProductType()" because "product" is null
at org.compiere.acct.Doc_Inventory.createFacts(Doc_Inventory.java:249)
at org.compiere.acct.Doc.postLogic(Doc.java:741)
at org.compiere.acct.Doc.post(Doc.java:625)

@dpansheriya dpansheriya deleted the IDEMPIERE-5836 branch May 20, 2024 06:58
@dpansheriya dpansheriya mentioned this pull request May 20, 2024
Open
16 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CarlosRuiz-globalqss CarlosRuiz-globalqss CarlosRuiz-globalqss requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@dpansheriya @CarlosRuiz-globalqss