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

[14.0][IMP] account_asset_management, allow manual entry/editing of depreciation board #1733

Open
wants to merge 3 commits into
base: 14.0
Choose a base branch
from
Open

[14.0][IMP] account_asset_management, allow manual entry/editing of depreciation board #1733

wants to merge 3 commits into from
+134 −9

Conversation

bosd
Copy link
Contributor

@bosd bosd commented Sep 30, 2023

Forward port of #1208 continuation of #1237

@bosd
Copy link
Contributor Author

bosd commented Oct 7, 2023

@Dranyel-Bosd Can you please review?

Dranyel-Bosd
Dranyel-Bosd approved these changes Oct 17, 2023
View reviewed changes
Copy link

@Dranyel-Bosd Dranyel-Bosd left a comment

Choose a reason for hiding this comment

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

LGTM 👍 🎇

bofiltd
bofiltd approved these changes Nov 27, 2023
View reviewed changes
Copy link

@bofiltd bofiltd left a comment

Choose a reason for hiding this comment

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

LGTM

@bosd
Copy link
Contributor Author

bosd commented Feb 3, 2024

@etobella Can you please merge?

@bosd bosd force-pushed the 14.0-asset-man-line branch from 5b74761 to c41e49e Compare February 3, 2024 21:00
@bosd
Copy link
Contributor Author

bosd commented Feb 3, 2024

Improved coverage
@etobella it is ready now

@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). 🤖

etobella
etobella reviewed Feb 3, 2024
View reviewed changes
Copy link
Member

@etobella etobella left a comment

Choose a reason for hiding this comment

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

Technically LGTM, however I will ask comments from @JordiBForgeFlow as original author of the change 😉

@bosd
Copy link
Contributor Author

bosd commented Mar 7, 2024

ping @JordiBForgeFlow Can you have a look here / merge? 🙏

@etobella
Copy link
Member

etobella commented Mar 7, 2024

/ocabot merge patch

@etobella
Copy link
Member

etobella commented Mar 7, 2024

Let's proceed with merge in order to avoid delay (we have given enough time)

@pedrobaeza pedrobaeza added this to the 14.0 milestone Mar 7, 2024
pedrobaeza
pedrobaeza requested changes Mar 7, 2024
View reviewed changes
Copy link
Member

@pedrobaeza pedrobaeza left a comment

Choose a reason for hiding this comment

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

Sorry for going late to the party, but I don't think we should merge as is.

  • The number of commits is not acceptable.
  • The implementation with so many new fields is not good.

I'm aborting the merge and check this deeply proposing the implementation today.

@OCA-git-bot
Copy link
Contributor

On my way to merge this fine PR!
Prepared branch 14.0-ocabot-merge-pr-1733-by-etobella-bump-patch, awaiting test results.

OCA-git-bot added a commit that referenced this pull request Mar 7, 2024
@OCA-git-bot
Merge PR #1733 into 14.0
1250935 
Signed-off-by etobella
@OCA-git-bot
Copy link
Contributor

@etobella your merge command was aborted due to failed check(s), which you can inspect on this commit of 14.0-ocabot-merge-pr-1733-by-etobella-bump-patch.

After fixing the problem, you can re-issue a merge command. Please refrain from merging manually as it will most probably make the target branch red.

luc-demeyer
luc-demeyer reviewed Mar 7, 2024
View reviewed changes
Copy link
Contributor

@luc-demeyer luc-demeyer left a comment

Choose a reason for hiding this comment

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

I am not sure if we really need this.
The depreciation table can already be edited before confirming an asset.
I also think we'll hit performance probelems for companies with a large niumber of assets by changing the store=true on a couple of fields (my largest customer has close to 100000 assets, but ecosoft has a customer with more)
@Saran440 what do you think ?

@bosd
Copy link
Contributor Author

bosd commented Apr 22, 2024

The depreciation table can already be edited before confirming an asset.

But we cannot edit a running asset. Which is quite common to do in some countries.
So, I think we need this.

@JordiBForgeFlow @kittiu What do you think?

@JordiBForgeFlow
Copy link
Member

@bosd I also think that we need this.

@bosd
Copy link
Contributor Author

bosd commented Apr 22, 2024

I'm aborting the merge and check this deeply proposing the implementation today.

@pedrobaeza Which changes would you like to see?

@kittiu
Copy link
Member

kittiu commented Apr 23, 2024

@bosd IMHO I think the current one that edit before submit is good enough. I think I am too concern with performance when dealing with lots of asset. Will this store false account into it?

If not, I think this can be a good feature.

@bosd bosd force-pushed the 14.0-asset-man-line branch from c41e49e to 8f89c5a Compare April 28, 2024 10:07
JordiBForgeFlow and others added 3 commits April 28, 2024 12:40
@JordiBForgeFlow @bosd
[IMP] account_asset_management: allow manual entry/editing of depreci…
8bd3651 
…ation board.

- Enter manually the depreciation board.
- Compute the depreciation board based on rules, but allow editing of lines.

Some companies may need to manage a custom depreciation board because the depreciation schedule is too custom,
or because it may have evolved over time and cannot follow a single method.
In those cases it's better to manage a manual depreciation schedule.

[account_asset_management][fix] allow updating the previous_id

[account_asset_management][fix] comparison of dates
@kittiu @bosd
[IMP] account_asset_management: improve tests, manual depreciation co…
4859740 
…nstraints
@bosd
[IMP] account_asset_management: simplify,refactor manual depreciation
4301ea4
@bosd bosd force-pushed the 14.0-asset-man-line branch from 8f89c5a to 4301ea4 Compare April 28, 2024 10:55
@bosd
Copy link
Contributor Author

bosd commented Apr 28, 2024

Update: Changed the fields back to stored.

  • The number of commits is not acceptable.

I have squashed the commits of @JordiBForgeFlow .
The other commits I kept in place to retain author ship.

Renamed the commit messages.

@bosd
Copy link
Contributor Author

bosd commented Apr 29, 2024

@pedrobaeza Is this one ok now?

@github-actions GitHub Actions
Copy link

github-actions bot commented Oct 6, 2024

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

@github-actions github-actions bot added the stale PR/Issue without recent activity, it'll be soon closed automatically. label Oct 6, 2024
@bosd bosd requested a review from pedrobaeza October 10, 2024 20:18
@github-actions github-actions bot removed the stale PR/Issue without recent activity, it'll be soon closed automatically. label Oct 13, 2024
@pedrobaeza
Copy link
Member

I'm trying this, and it's very confusing to put manual depreciation board, add a line, and the error "You should at least have a starting depreciation line of Type 'Depreciation Base'." to appear, getting to a blocked situation, as that line can't be put manually nor computed.

Another problem is the previous_id computation, using compute hacks + double writing. Please change the approach. One possibility is to add the assets to "recompute its children", and then write all the previous_id.

Saran440
Saran440 reviewed Oct 31, 2024
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.

@bosd Functional Test.

When we use Allow editing the depreciation board, Days in depre will not calculated.

Selection_001

@bosd
Copy link
Contributor Author

bosd commented Oct 31, 2024

@bosd Functional Test.

When we use Allow editing the depreciation board, Days in depre will not calculated.

Imo that is a design intent. As when one does a manual depreciation it is not distributed evenly among the terms.
@kittiu is that how you originally intended this?

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

@Saran440 Saran440 Saran440 left review comments

@luc-demeyer luc-demeyer luc-demeyer left review comments

@Dranyel-Bosd Dranyel-Bosd Dranyel-Bosd approved these changes

@bofiltd bofiltd bofiltd approved these changes

@etobella etobella etobella approved these changes

@pedrobaeza pedrobaeza Awaiting requested review from pedrobaeza

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
14.0
Development

Successfully merging this pull request may close these issues.
10 participants
@bosd @OCA-git-bot @etobella @JordiBForgeFlow @kittiu @pedrobaeza @luc-demeyer @Saran440 @bofiltd @Dranyel-Bosd