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.

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

[14.0][ADD] account_move_import_queue_context #654

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

[14.0][ADD] account_move_import_queue_context #654

wants to merge 1 commit into from

Conversation

toita86
Copy link

@toita86 toita86 commented May 24, 2024

No description provided.

@toita86 toita86 force-pushed the 14.0-ADD-account_move_import_queue_context branch from 8a630c2 to 28c5f09 Compare May 24, 2024 15:58
amh-mw
amh-mw suggested changes May 27, 2024
View reviewed changes
Copy link
Contributor

@amh-mw amh-mw left a comment

Choose a reason for hiding this comment

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

LGTM, but cover with unit test?

@toita86 toita86 force-pushed the 14.0-ADD-account_move_import_queue_context branch from 28c5f09 to 6bf2c11 Compare May 28, 2024 16:53
@toita86
Copy link
Author

toita86 commented May 28, 2024

Just added, let me know if I'm missing something.

@francesco-ooops
Copy link
Contributor

@amh-mw good for you?

aleuffre
aleuffre approved these changes May 29, 2024
View reviewed changes
Copy link

@aleuffre aleuffre 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

account_move_import_queue_context/tests/test_account_move_import_queue_context.py Outdated

class TestJobPrepareContext(TransactionCase):
def test_job_prepare_context_before_enqueue_keys(self):
model = self.env["base"].create({})
Copy link

Choose a reason for hiding this comment

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

question(non-blocking): do you need this create? Couldn't you just do self.env["base"]._job_prepare_context_before_enqueue_keys() ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I concur the create is unnecessary for methods decorated @api.model.

amh-mw
amh-mw approved these changes May 30, 2024
View reviewed changes
account_move_import_queue_context/tests/test_account_move_import_queue_context.py Outdated

class TestJobPrepareContext(TransactionCase):
def test_job_prepare_context_before_enqueue_keys(self):
model = self.env["base"].create({})
Copy link
Contributor

Choose a reason for hiding this comment

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

I concur the create is unnecessary for methods decorated @api.model.

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

@toita86 toita86 force-pushed the 14.0-ADD-account_move_import_queue_context branch from 6bf2c11 to 8f3b12b Compare May 31, 2024 13:08
@francesco-ooops
Copy link
Contributor

@pedrobaeza good to go?

@pedrobaeza pedrobaeza added this to the 14.0 milestone Jun 3, 2024
@pedrobaeza
Copy link
Member

I feel here there's a missing piece, as with this module, you can't import directly with queue_job. I also don't understand why going to such complex architecture, and more when Odoo has improved the import in future versions.

@github-actions GitHub Actions
Copy link

github-actions bot commented Oct 6, 2024

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

@github-actions github-actions bot added the stale PR/Issue without recent activity, it'll be soon closed automatically. label Oct 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@amh-mw amh-mw amh-mw approved these changes

@aleuffre aleuffre aleuffre approved these changes

Assignees
No one assigned
Labels
approved ready to merge stale PR/Issue without recent activity, it'll be soon closed automatically.
Projects
None yet
Milestone
14.0
Development

Successfully merging this pull request may close these issues.
6 participants
@toita86 @francesco-ooops @OCA-git-bot @pedrobaeza @amh-mw @aleuffre