Skip to content

[IMP] accounting: reconciliation models update #5943

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
Closed
wants to merge 1 commit into from
Closed

[IMP] accounting: reconciliation models update #5943

wants to merge 1 commit into from

Conversation

@Donapi
Copy link
Contributor

@Donapi Donapi commented Sep 29, 2023

taskid-3377307

@robodoo
Copy link
Collaborator

robodoo commented Sep 29, 2023

Pull request status dashboard.

@C3POdoo C3POdoo requested a review from a team September 29, 2023 09:44
@Donapi Donapi force-pushed the 16.0-reconciliation-models-dopi branch 2 times, most recently from 7f79b0c to 61ffcfc Compare October 3, 2023 09:40
@C3POdoo C3POdoo requested a review from a team October 3, 2023 10:04
dade-odoo
dade-odoo requested changes Oct 3, 2023
View reviewed changes
Copy link
Contributor

@dade-odoo dade-odoo left a comment

Choose a reason for hiding this comment

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

Thanks for taking over on this one while I was out @Donapi :) it looks great!

I made just a few comments. I like your choice to explain the default reconciliation models, but I'm noticing that that does change based on the fiscal localization (at least in the runbot I'm looking at). I'm not sure if that's a problem or not - maybe whoever does the final review can share their opinion.

I think it's also important to explain how to create your own model so that users can fully understand them. I hope that my additions in your "Reconciliation model types" section is enough to explain that, but it may be worth expanding on that even more. Let me know what you think.

@Donapi Donapi force-pushed the 16.0-reconciliation-models-dopi branch from 61ffcfc to 1254374 Compare October 3, 2023 14:17
@Donapi Donapi requested a review from dade-odoo October 3, 2023 14:19
@jcs-odoo jcs-odoo mentioned this pull request Oct 4, 2023
Closed
jcs-odoo
jcs-odoo reviewed Oct 4, 2023
View reviewed changes
Copy link
Contributor

@jcs-odoo jcs-odoo left a comment

Choose a reason for hiding this comment

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

Hello, just a quick comment about the structure, but I haven't reviewed the content.

Why do you move the reconciliation models doc? I would rather not.
We tried to avoid sublevels as much as possible during the accounting structure refactoring.

In any case, such changes in the structure should be apart from the content improvement, to keep it cleaner. Typically, it would be applied from 15.0, in a separate commit.

@jcs-odoo jcs-odoo requested a review from a team October 4, 2023 10:46
@Donapi
Copy link
Contributor Author

Donapi commented Oct 5, 2023

Hello, just a quick comment about the structure, but I haven't reviewed the content.

Why do you move the reconciliation models doc? I would rather not. We tried to avoid sublevels as much as possible during the accounting structure refactoring.

In any case, such changes in the structure should be apart from the content improvement, to keep it cleaner. Typically, it would be applied from 15.0, in a separate commit.

@jcs-odoo, @xpl-odoo suggested moving this page or including the topic directly in the Reconciliation doc. I didn't like the last option as I thought the Reconciliation doc page would be too long, so we decided to create a sublevel. We can reconsider it if needed.

@jcs-odoo
Copy link
Contributor

jcs-odoo commented Oct 6, 2023

Thanks for the answer @Donapi

I just skim-read your doc, and we're missing all the juicy parts with the conditions and the partner mapping. That's exactly the kind of information we need on this page. For the "match regex" part, just explain briefly what a regex is, add a link to a good page explaining it (preferably an open doc, a wiki, or something like that) and add an example (you can reuse the one from the demo data and explain it).

Thanks and good luck :)

@Donapi Donapi force-pushed the 16.0-reconciliation-models-dopi branch from 1254374 to bbad7cf Compare October 10, 2023 11:35
@xpl-odoo xpl-odoo requested a review from dade-odoo December 6, 2023 09:33
dade-odoo
dade-odoo approved these changes Dec 7, 2023
View reviewed changes
Copy link
Contributor

@dade-odoo dade-odoo left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Hi @Donapi :)
This looks great to me. I think your regex and partner mapping sections are great additions. I just made one suggestion to mention that the default models change depending on your fiscal localization. Otherwise I think it's ready for an r+ @xpl-odoo

@Donapi Donapi force-pushed the 16.0-reconciliation-models-dopi branch from bbad7cf to 6409ad7 Compare December 14, 2023 13:05
xpl-odoo
xpl-odoo approved these changes Jan 3, 2024
View reviewed changes
Copy link
Contributor

@xpl-odoo xpl-odoo left a comment

Choose a reason for hiding this comment

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

@robodoo r+

@robodoo
Copy link
Collaborator

robodoo commented Jan 3, 2024

@Donapi @xpl-odoo staging failed: ci/documentation (view more at https://runbot.odoo.com/runbot/build/55896384)

@xpl-odoo
Copy link
Contributor

xpl-odoo commented Jan 3, 2024

@robodoo retry

@robodoo
Copy link
Collaborator

robodoo commented Jan 3, 2024

@Donapi @xpl-odoo staging failed: ci/documentation (view more at https://runbot.odoo.com/runbot/build/55899278)

@jcs-odoo
Copy link
Contributor

jcs-odoo commented Jan 4, 2024

robodoo retry

robodoo pushed a commit that referenced this pull request Jan 4, 2024
@Donapi
[IMP] accounting: reconciliation models update
24b5c90 
taskid-3377307

closes #5943

Signed-off-by: Xavier Platteau (xpl) <[email protected]>
@robodoo
Copy link
Collaborator

robodoo commented Jan 4, 2024

@Donapi @xpl-odoo staging failed: ci/documentation (view more at https://runbot.odoo.com/runbot/build/55942034)

@jcs-odoo
Copy link
Contributor

jcs-odoo commented Jan 4, 2024

@Donapi @xpl-odoo we were doing retry without checking the build: it seems the labels don't work anymore. Maybe something that was merged in the meantime? It's weird though. But then the branch needs to be rebased and fixed.

@xpl-odoo xpl-odoo force-pushed the 16.0-reconciliation-models-dopi branch 2 times, most recently from 936dc92 to 143fa54 Compare January 22, 2024 14:11
@xpl-odoo xpl-odoo force-pushed the 16.0-reconciliation-models-dopi branch from 143fa54 to 7bdcde5 Compare January 22, 2024 14:20
@xpl-odoo
Copy link
Contributor

xpl-odoo commented Jan 22, 2024

@robodoo r+

robodoo pushed a commit that referenced this pull request Jan 22, 2024
@Donapi @xpl-odoo
[IMP] accounting: reconciliation models update
9f09422 
task-3377307

closes #5943

Signed-off-by: Xavier Platteau (xpl) <[email protected]>
@robodoo robodoo closed this Jan 22, 2024
This was referenced Jan 22, 2024
Closed
Closed
Closed
Closed
Closed
Closed
Closed
@fw-bot
Copy link
Collaborator

fw-bot commented Jan 26, 2024

@Donapi @xpl-odoo this pull request has forward-port PRs awaiting action (not merged or closed):

4 similar comments
@fw-bot
Copy link
Collaborator

fw-bot commented Jan 27, 2024

@Donapi @xpl-odoo this pull request has forward-port PRs awaiting action (not merged or closed):

@fw-bot
Copy link
Collaborator

fw-bot commented Jan 28, 2024

@Donapi @xpl-odoo this pull request has forward-port PRs awaiting action (not merged or closed):

@fw-bot
Copy link
Collaborator

fw-bot commented Jan 29, 2024

@Donapi @xpl-odoo this pull request has forward-port PRs awaiting action (not merged or closed):

@fw-bot
Copy link
Collaborator

fw-bot commented Jan 30, 2024

@Donapi @xpl-odoo this pull request has forward-port PRs awaiting action (not merged or closed):

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

Reviewers

@jcs-odoo jcs-odoo jcs-odoo left review comments

@xpl-odoo xpl-odoo xpl-odoo approved these changes

@dade-odoo dade-odoo dade-odoo approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@Donapi @robodoo @jcs-odoo @xpl-odoo @fw-bot @dade-odoo