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

[16.0][FIX] document_page_tag: Use api.model_create_multi #491

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

Conversation

anusriNPS
Copy link

Using decorator api.model_create_multi for create method in document_page_tag model in order to avoid below warning:

WARNING devel odoo.api.create: The model odoo.addons.document_page_tag.models.document_page_tag is not overriding the create method in batch

@anusriNPS anusriNPS marked this pull request as draft July 16, 2024 08:25
@anusriNPS anusriNPS force-pushed the 16.0-document-tag-warn branch 2 times, most recently from d83332f to 6abb713 Compare July 16, 2024 09:21
@anusriNPS anusriNPS marked this pull request as ready for review July 16, 2024 09:23
Comment on lines 18 to 27
@api.model_create_multi
def create(self, vals_list):
"""Be nice when trying to create duplicates"""
existing = self.search([("name", "=ilike", vals["name"])], limit=1)
if existing:
return existing
return super().create(vals)
for vals in vals_list:
existing = self.search([("name", "=ilike", vals["name"])], limit=1)
if existing:
return existing
return super().create(vals_list)

Choose a reason for hiding this comment

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

Issue: When trying to create two tags at the same time, one that exists and one that doesn't, we return the one tag that does exist and don't create the one that doesn't.

Copy link
Author

Choose a reason for hiding this comment

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

Changes included to handle tag creation with one exist and not exist at the same time

Copy link

@PicchiSeba PicchiSeba left a comment

Choose a reason for hiding this comment

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

Code Review: generally seems good. The only issue I have is with the variable names in the test (not your fault, they were already done like this).
We might consider converting them to snake_case format

document_page_tag/tests/test_document_page_tag.py Outdated Show resolved Hide resolved
@PicchiSeba
Copy link

watch out for the pre-commit error at line 68 in document_page_tag/tests/test_document_page_tag.py

   Using decorator api.model_create_multi for create
method in document_page_tag model
Copy link

@PicchiSeba PicchiSeba left a comment

Choose a reason for hiding this comment

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

Code review: 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). 🤖

@max3903 max3903 added this to the 16.0 milestone Sep 21, 2024
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