Skip to content

[FIX] agreement_legal: copy agreement not get the current agreement I… - #133

Merged
OCA-git-bot merged 1 commit into
OCA:18.0from
APSL:18.0-fix-agreement_legal
Sep 3, 2026
Merged

[FIX] agreement_legal: copy agreement not get the current agreement I…#133
OCA-git-bot merged 1 commit into
OCA:18.0from
APSL:18.0-fix-agreement_legal

Conversation

@lbarry-apsl

Copy link
Copy Markdown
Member

When an agreement is copied from the template, the temp_agreement_id does not pick up the ID of the new agreement, which makes it impossible to select the section of the created agreement.

cc https://github.com/APSL

@shirashi3771 @palomagrc93 @miquelalzanillas @peluko00 @BernatObrador @cubells @miquelpascual @ppyczko @isugac @javierobcn please review.

@OCA-git-bot

Copy link
Copy Markdown
Contributor

Hi @max3903, @ygol,
some modules you are maintaining are being modified, check this out!

@max3903 max3903 added this to the 18.0 milestone Aug 31, 2026
@max3903 max3903 self-assigned this Aug 31, 2026
Comment thread agreement_legal/models/agreement.py Outdated
@@ -487,6 +487,7 @@ def copy(self, default=None):
section_map[section.id] = new_section.id
for clause in self.clauses_ids:
values = {"agreement_id": res.id}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
values = {"agreement_id": res.id}
values = {
"agreement_id": res.id,
"temp_agreement_id": res.id,
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done @cubells , thanks

@lbarry-apsl
lbarry-apsl force-pushed the 18.0-fix-agreement_legal branch from eb2971c to 4254c50 Compare September 1, 2026 07:18
@lbarry-apsl
lbarry-apsl requested a review from cubells September 1, 2026 07:27

@cubells cubells left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍

LGTM

Code review and test

@miquelpascual miquelpascual left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM!

@lbarry-apsl

Copy link
Copy Markdown
Member Author

Hi @max3903 , can be merge?

@max3903

max3903 commented Sep 1, 2026

Copy link
Copy Markdown
Member

Thanks for the fix — the copy() change looks correct for the reported issue (temp_agreement_id must point at the new agreement so the section domain works).

One related gap: recompute_from_template still copies clauses with only agreement_id set:

https://github.com/OCA/agreement/blob/18.0/agreement_legal/models/agreement.py#L297-L301

That path has the same symptom after “reset from template”. Could you apply the same temp_agreement_id assignment there (and ideally a short regression test that copied/recomputed clauses have temp_agreement_id == agreement_id)?

Otherwise LGTM for merging the copy fix as-is and doing that as a tiny follow-up.

@lbarry-apsl

Copy link
Copy Markdown
Member Author

Thanks for the fix — the copy() change looks correct for the reported issue (temp_agreement_id must point at the new agreement so the section domain works).

One related gap: recompute_from_template still copies clauses with only agreement_id set:

https://github.com/OCA/agreement/blob/18.0/agreement_legal/models/agreement.py#L297-L301

That path has the same symptom after “reset from template”. Could you apply the same temp_agreement_id assignment there (and ideally a short regression test that copied/recomputed clauses have temp_agreement_id == agreement_id)?

Otherwise LGTM for merging the copy fix as-is and doing that as a tiny follow-up.

Sure @max3903 , no problem, I'll add the other correction and the test

@lbarry-apsl
lbarry-apsl force-pushed the 18.0-fix-agreement_legal branch from 4254c50 to 882803c Compare September 3, 2026 10:08
@lbarry-apsl

Copy link
Copy Markdown
Member Author

Hi @max3903 , I've already made the changes, can you check?

@max3903

max3903 commented Sep 3, 2026

Copy link
Copy Markdown
Member

/ocabot merge patch

@OCA-git-bot

Copy link
Copy Markdown
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 18.0-ocabot-merge-pr-133-by-max3903-bump-patch, awaiting test results.

@OCA-git-bot
OCA-git-bot merged commit 83f19ee into OCA:18.0 Sep 3, 2026
7 checks passed
@OCA-git-bot

Copy link
Copy Markdown
Contributor

Congratulations, your PR was merged at d337a0b. 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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants