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.

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

[17.0][ADD] partner_title_active #1869

Open
wants to merge 1 commit into
base: 17.0
Choose a base branch
from

Conversation

yankinmax
Copy link
Contributor

Adds an Active field to the partner title model to be able to deactivate titles without deleting them

Copy link

@imlopes imlopes left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

@vvrossem vvrossem left a comment

Choose a reason for hiding this comment

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

thank you for this contribution to the OCA 🙏

TestPartnerTitle should be adapted as it doesn't assert if the title is active or not

Comment on lines 13 to 17
def test_01_partner_title(self):
partner = self.Partner.create({"name": "A Partner"})
self.assertTrue(partner.active, "Partner should be active by default")
partner.action_archive()
self.assertFalse(partner.active, "Partner should be inactive after archiving")

Choose a reason for hiding this comment

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

This doesn't test whether the title is active or not 😕

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why? Can you describe pls a test scenario you think is more suitable?

Choose a reason for hiding this comment

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

The partner that is created doesn't have a title set.
The scenario might be:

  1. create a partner with a title (res.partner.title)
  2. test the title is active
  3. archive the title, assert it is not active

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, I'll adapt, thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ahaha @vvrossem , I've just get that I was archiving the partner and not the title 🥲
good catch, shame on me!

@yankinmax yankinmax force-pushed the add-partner_title_active branch 2 times, most recently from d057e9f to afeaeea Compare November 1, 2024 11:04
@Abimael1321
Copy link

LGTM

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

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.

6 participants