[14.0][FIX] delivery_auto_refresh

[toita86]

backport of #799

[OCA-git-bot]

Hi @aleuffre, @PicchiSeba, @renda-dev,
some modules you are maintaining are being modified, check this out!

[francesco-ooops]

@jbaudoux would you mind reviewing this backporting? thanks!

[PicchiSeba]

Code Review:

That's a lot of work. Kudos. The changes to adapt to 14.0 look fine to me.

[jbaudoux]

LGTM

[jbaudoux]

cc @pedrobaeza

[OCA-git-bot]

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

[toita86]

Can you please squash a bit the commit history?

@pedrobaeza
Of course, do you have any suggestion? I'm not to sure on how to do it.

[francesco-ooops]

Or rather, which commits to squash together

[rousseldenis]

I would say commits should remain as it is. They reflect development flow and are the same on the other main branch (even if they should have been a little bit enhanced in their messages).

[pedrobaeza]

No, they don't add value when merged. The commit "delivery_auto_refresh: fix create in batch" is not adding value to have it separated. The code itself speaks by itself. And they are not following commit message guidelines:

https://github.com/OCA/odoo-community.org/blob/master/website/Contribution/CONTRIBUTING.rst#commit-message

[francesco-ooops]

@pedrobaeza can you please tell us in detail which ones should be squashed in your opinion, so we can proceed?

[rousseldenis]

@jbaudoux

[rousseldenis]

@pedrobaeza I would say you had merged it yousrself like this : #799 (review)

[pedrobaeza]

IMO, you can pack together improvements which code speaks by itself, summarizing in the commit message all of them. At the end, what we want when we do a blame, is to identify the reason of the change. But if there are several commits touching the same lines of code, at the end, the blame operation is a hell.

The same way, a commit that is not telling the reason behind, doesn't have value too much value. Take this commit:

4785c77

With the commit message delivery_auto_refresh: method copied from 16.0. Which value brings to have it separated if it's not bringing any useful information, and even more, it's not being called in any place in the same commit!!! It may be interesting to keep it as a separated commit if at least where it's used is packed in the same commit and there's an explanation of why this is done.

I know that it seems comfortable to do continuously git commit to "save the current work", but at the end, that doesn't help when reviewing code (as our development cycle may not respond the feature cycle), and also for grouping changes in a logical way.

For reference, what I do to save the work is usually git commit --amend. If working in several features at the same time, I put several commits, and later changes are committed with --fixup <sha> for merging them before pushing with the proper original commits.

[pedrobaeza]

@pedrobaeza I would say you had merged it yousrself like this : #799 (review)

Yes, as said in other thread, I don't fight all the possible battles. I have to resign in some of them. In this case, it was my fault to block the PR a lot of time and I already asked for a lot of changes, so I didn't want to be even more picky about that.

In this case, it's also not the most important thing, as this is an old branch, but the number of commits have increased (from 11 to 16), and it can be an opportunity to converge in commit strategies.

[francesco-ooops]

@pedrobaeza in the case of commit delivery_auto_refresh: method copied from 16.0 it was isolated in a specific commit to preserve odoo SA authorship. Should we also keep commit name/description, even if it's not the full content of the commit?

[pedrobaeza]

Good that you have arisen this, as I wasn't really aware (didn't check this PR in content yet). Starting with, you can't do that, as this module is AGPL, and the original code is LGPL. More in the formal aspect, you are putting that method, but not indicating anything in the commit message saying that this is copied from Odoo code, and not putting in the same commit the changes on the README Part of this module is a backport.... That's what I refer about packing things in a logical way in the commits, and also to document things properly for helping others (or ourselves in the future) to understand why things are there.

About the code itself, I don't think the prepare method is bringing too much value, and even if so, you can just put it without that attribution concern, as at the end, it's not a specific secret technique to do the code, and the lines are obvious even and will be the same if you do it by hand, and for sure you need later adjustments for version 14.

[PicchiSeba]

Code review: Remove all references of the 17.0 migration. This should not be a concern to us because this is a backport and this matter should be resolved upstream

[toita86]

@pedrobaeza I'd appreciate any advice or suggestions on how to handle this more efficiently, or if there's a better approach to resolve this circular dependency.

I'm currently facing a circular dependency issue between two PRs:

#849: delivery_auto_refresh
OCA/sale-promotion#224: sale_coupon_delivery_auto_refresh

The issue arises because sale_coupon_delivery_auto_refresh has an unreleased dependency on delivery_auto_refresh, and this creates a circular reference between the two modules. The fixes introduced in both PRs need each other to avoid breaking the logic.

To resolve this, my idea is to:

• Remove the unreleased dependency from sale_coupon_delivery_auto_refresh PR temporarily, allowing us to merge the delivery_auto_refresh PR first.

• After merging delivery_auto_refresh, we can merge sale_coupon_delivery_auto_refresh, which includes the fix for the dependency issue between the two modules.

Does this approach make sense, or would you recommend a different solution? I’d appreciate your input!

[pedrobaeza]

Yes, remove here the temporary dependency, as I don't see too much sense in adding it while the target module is not being used here.

[toita86]

Yes, remove here the temporary dependency, as I don't see too much sense in adding it while the target module is not being used here.

Thanks for the suggestion! I'll go ahead and remove the temporary dependency.