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

[15.0][ENH] account_asset_management: default salvage value #1567

Merged
merged 1 commit into from
Feb 15, 2024
Merged

[15.0][ENH] account_asset_management: default salvage value #1567

merged 1 commit into from
Feb 15, 2024

Conversation

ps-tubtim
Copy link
Member

This PR adds the field Salvage Value in the asset profile for default salvage value when creating the asset.

Example

  1. Set Salvage Value in the asset profile
  2. Create an asset with an asset profile
  3. Salvage Value in asset changes following Salvage Value set in the asset profile

Selection_038

asset

@ps-tubtim ps-tubtim force-pushed the 15.0-enh-account_asset_managemnt-salvage_value branch from c213b38 to b5c4c42 Compare January 26, 2023 07:44
Saran440
Saran440 approved these changes May 18, 2023
View reviewed changes
Copy link
Member

@Saran440 Saran440 left a comment

Choose a reason for hiding this comment

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

Functional Test 👍

@Saran440 Saran440 force-pushed the 15.0-enh-account_asset_managemnt-salvage_value branch from b5c4c42 to 6f9db98 Compare September 26, 2023 07:14
@ps-tubtim ps-tubtim force-pushed the 15.0-enh-account_asset_managemnt-salvage_value branch from 6f9db98 to 3a0d847 Compare October 26, 2023 08:53
@ps-tubtim ps-tubtim force-pushed the 15.0-enh-account_asset_managemnt-salvage_value branch from 3a0d847 to aedd8ed Compare January 18, 2024 02:45
@Saran440
Copy link
Member

Hello @OCA/accounting-maintainers,

I would like your feedback on this pull request.
Is this a generic case where users receive many assets and don't want to add salvage values one by one?
if it not generic, we are open to moving it to the l10n repo.

@pedrobaeza
Copy link
Member

Hi, Saran, I think this can be more valuable if expressed in percentage instead. I don't know any case where a fixed value is required.

@luc-demeyer
Copy link
Contributor

@Saran440
I haven't encountered this demand yet, PR looks techical ok to me.
Since I haven't encountered this use case yet I can also not judge on percentage versus value, can you document your specific use case ?

@Saran440 Saran440 force-pushed the 15.0-enh-account_asset_managemnt-salvage_value branch 2 times, most recently from 14bae7e to 34900cf Compare February 15, 2024 04:45
@Saran440
Copy link
Member

@pedrobaeza @luc-demeyer Thank you for your advise

Now, I change salvage value in asset profile can config with percentage or fixed amount.

For use case:

  • When purchasing many assets and configured asset profile with Create an asset by product item,
    it will create many assets. so, accountant must change salvage value each asset. it will take time.
    So, I think salvage value should config in asset profile will help user for this case

  • Accounting standard of our country cannot set salvage value as zero (Not sure about other country).

  • Other ERP (Frappe) can config with percent like asset profile. I think it useful for user.

@Saran440 Saran440 force-pushed the 15.0-enh-account_asset_managemnt-salvage_value branch 2 times, most recently from 27fd808 to 6f96ba9 Compare February 15, 2024 05:41
@Saran440 Saran440 mentioned this pull request Feb 15, 2024
Merged
@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

@pedrobaeza pedrobaeza added this to the 15.0 milestone Feb 15, 2024
pedrobaeza
pedrobaeza reviewed Feb 15, 2024
View reviewed changes
account_asset_management/models/account_asset.py Outdated
@@ -446,7 +461,7 @@ def create(self, vals):
asset = super().create(vals)
if self.env.context.get("create_asset_from_move_line"):
# Trigger compute of depreciation_base
asset.salvage_value = 0.0
asset.salvage_value = asset._get_salvage_value_profile()
Copy link
Member

Choose a reason for hiding this comment

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

One doubt: is this really needed? If you remove these lines, I think it will work with the new compute.

Copy link
Member

Choose a reason for hiding this comment

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

Thank you for your review. I have already deleted it as it was not needed.

@ps-tubtim @Saran440
[ENH] account_asset_management: default salvage value on profile
ad4fbed 
This PR enables the configuration of salvage value in the asset profile.
Users can configure it with a fixed amount or a percentage of the salvage value.
@Saran440 Saran440 force-pushed the 15.0-enh-account_asset_managemnt-salvage_value branch from 6f96ba9 to ad4fbed Compare February 15, 2024 11:55
@pedrobaeza
Copy link
Member

/ocabot merge minor

@OCA-git-bot
Copy link
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 15.0-ocabot-merge-pr-1567-by-pedrobaeza-bump-minor, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit a736c13 into OCA:15.0 Feb 15, 2024
7 checks passed
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at 5c38b65. Thanks a lot for contributing to OCA. ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Saran440 Saran440 Saran440 approved these changes

@pedrobaeza pedrobaeza pedrobaeza approved these changes

Assignees
No one assigned
Labels
approved merged 🎉 ready to merge
Projects
None yet
Milestone
15.0
Development

Successfully merging this pull request may close these issues.
5 participants
@ps-tubtim @Saran440 @pedrobaeza @luc-demeyer @OCA-git-bot