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.

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

Add Inline landed costing feature #31

Draft
wants to merge 10 commits into
base: version-14
Choose a base branch
from
Draft

Add Inline landed costing feature #31

wants to merge 10 commits into from

Conversation

HKuz
Copy link
Collaborator

@HKuz HKuz commented Sep 1, 2023

Addresses #4

Opening PR to track progress - for now I see this as two 'phases' - 1) get the feature working on a standalone basis in v14 and 2) integrate the feature to work with WO subcontracting.

PHASE I

  • Update Inventory Tools Settings, PR, and PI schemas to include new necessary fields
  • Purchase Receipt:
    • Add setting-based server-side functionality to custom Inv tools Purchase Receipt
    • Port JS/UI functionality to distribute landed costs and show/hide relevant columns
  • Purchase Invoice:
    • Add setting-based server-side functionality to custom Inv tools Purchase Invoice
    • Port JS/UI functionality to distribute landed costs and show/hide relevant columns
  • Testing (done manually for now) to see if everything working as expected:
    • ERPNext current functionality still works: Turn enable inline LC feature OFF, submit PR, create LCV
    • ERPNext current functionality still works: Turn enable inline LC feature ON, submit PR, create LCV
    • ERPNext current functionality still works: Cancel LCV in prior test
    • Feature: With feature ON, submit PR with inline LCs
    • Feature: With feature ON, submit PI with update stock checked and with inline LCs
    • Feature: With feature ON, submit a PR, create a PI off it (so can't check update stock), add inline LCs
    • Feature: Cancel the PI in prior test
    • Feature: With feature ON, submit PR with inline LCs, create a PI off it and add additional LCs
  • Documentation

PHASE II and tests -> moved to a separate issue

@agritheory agritheory marked this pull request as draft March 27, 2024 14:19
@agritheory agritheory changed the title DRAFT: add Inline landed costing feature Add Inline landed costing feature Mar 27, 2024
@agritheory
Copy link
Owner

@HKuz I tried to rebase this but I don't feel confident in the decisions I was making, can I get you to rebase this and we'll split the remaining work into separate ticket?

@HKuz
Copy link
Collaborator Author

HKuz commented Apr 16, 2024

@agritheory I rebased this, but in testing I realized at least one function needs to be updated for version-14 changes (it's trying to access a default account that isn't listed in Company settings anymore). I can ping you when this is updated and ready.

@agritheory
Copy link
Owner

@HKuz Just had a look at this again, we need to add docs but should probably split out the tests into another ticket.

@HKuz
Copy link
Collaborator Author

HKuz commented Apr 29, 2024

All code is now updated and re-tested for changes in ERPNext's underlying functions. In running through the test scenarios above, I added another one to create a PR with LCs, then create a PI off it with additional LCs. This uncovered a bug where it's double-counting the LCs when the PI is submitted.

In our JS code, when the Distribute Landed Costs Based On field is changed to either Qty or Amount, it's assuming everything in the taxes and charges table should be allocated (automatically sets the category to Valuation and Total and includes in the sum for the items table). It wasn't allowing the user to exclude a row (like sales tax in a PI) and recalculate the landed cost amount.

UPDATE - adjustments made in the javascript to let the user change the tax category and it triggers a re-calc of the landed cost amounts (excluding anything not marked as Valuation or Valuation and Total). Docs are also updated to note this.

For the double-counting solution I need to add a check in the PI for any LCs already in the PR so they're not double-counted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants