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

[16.0][ADD] New module l10n_be_bpost_address_validation #186

Open
wants to merge 13 commits into
base: 16.0
Choose a base branch
from
Open

[16.0][ADD] New module l10n_be_bpost_address_validation #186

wants to merge 13 commits into from

Conversation

kouffsamuel
Copy link
Contributor

This PR will allow to validate the partner's address with the API of bpost.

@kouffsamuel kouffsamuel force-pushed the 16.0-bpost-address-validation branch 2 times, most recently from db727c7 to fa27086 Compare April 18, 2023 06:25
@kouffsamuel kouffsamuel marked this pull request as ready for review April 18, 2023 07:29
@kouffsamuel kouffsamuel force-pushed the 16.0-bpost-address-validation branch from 926c5f8 to 0667bba Compare April 18, 2023 11:03
@kouffsamuel kouffsamuel changed the title [16.0][ADD] New module bpost_address_validation [16.0][ADD] New module l10n_be_bpost_address_validation May 2, 2023
@kouffsamuel kouffsamuel force-pushed the 16.0-bpost-address-validation branch from d95fa66 to 7e27e4f Compare May 2, 2023 09:08
@kouffsamuel kouffsamuel force-pushed the 16.0-bpost-address-validation branch from 7e27e4f to baeeed6 Compare May 2, 2023 09:11
lmignon
lmignon suggested changes May 19, 2023
View reviewed changes
Copy link
Contributor

@lmignon lmignon 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 great addon. Some comments but your are on the good way.

l10n_be_bpost_address_validation/wizards/bpost_address_validation.py Show resolved Hide resolved
l10n_be_bpost_address_validation/tests/test_res_partner.py Show resolved Hide resolved
l10n_be_bpost_address_validation/tests/test_bpost_address_validation_wizard.py Show resolved Hide resolved
l10n_be_bpost_address_validation/tests/__init__.py Outdated Show resolved Hide resolved
l10n_be_bpost_address_validation/models/res_partner.py Outdated Show resolved Hide resolved
l10n_be_bpost_address_validation/models/res_partner.py Show resolved Hide resolved
l10n_be_bpost_address_validation/wizards/bpost_address_validation.py
_name = "bpost.address.validation.wizard"

partner_id = fields.Many2one("res.partner", readonly=True)
is_valid = fields.Boolean(compute="_compute_response_address")
Copy link
Contributor

@lmignon lmignon May 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I'll submit a PR on your branch to simplify the code. Since all your fields are computed, they should be declared as is and Odoo will only call the compute method once for all the fields. Moreover, Your compute method should delcare dependecies on subfields of partner_id to ensure that when the street or zip or city fields are updated, the recompute is triggered.

see kouffsamuel#1

Copy link

@IT-Ideas IT-Ideas Nov 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I agree that writing on non compute fields in a compute is not recommended and that all these should be compute fields. I am ok with the PR @lmignon wrote. I juste left a comment on it on the implementation I would have done.

@kouffsamuel kouffsamuel force-pushed the 16.0-bpost-address-validation branch from a577957 to 40f128c Compare May 19, 2023 13:08
@lmignon
Copy link
Contributor

lmignon commented May 22, 2023

@kouffsamuel Can you take a look at kouffsamuel#1?

@github-actions GitHub Actions
Copy link

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 Nov 12, 2023
IT-Ideas
IT-Ideas reviewed Nov 13, 2023
View reviewed changes
Copy link

@IT-Ideas IT-Ideas left a comment

Choose a reason for hiding this comment

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

Thanks!

l10n_be_bpost_address_validation/tests/test_bpost_address_validation_wizard.py
Comment on lines +103 to +106

@classmethod
def tearDownClass(cls):
super(TestBpostAddressValidationWizard, cls).tearDownClass()
Copy link

Choose a reason for hiding this comment

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

Is this still needed?

l10n_be_bpost_address_validation/tests/test_res_partner.py
Comment on lines +34 to +37

@classmethod
def tearDownClass(cls):
super(TestResPartner, cls).tearDownClass()
Copy link

Choose a reason for hiding this comment

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

Is this still needed?

l10n_be_bpost_address_validation/wizards/bpost_address_validation.py
_name = "bpost.address.validation.wizard"

partner_id = fields.Many2one("res.partner", readonly=True)
is_valid = fields.Boolean(compute="_compute_response_address")
Copy link

@IT-Ideas IT-Ideas Nov 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I agree that writing on non compute fields in a compute is not recommended and that all these should be compute fields. I am ok with the PR @lmignon wrote. I juste left a comment on it on the implementation I would have done.

@github-actions github-actions bot closed this Dec 17, 2023
@sbidoul sbidoul reopened this Dec 17, 2023
@github-actions github-actions bot closed this Jan 21, 2024
@sbidoul sbidoul reopened this Jan 21, 2024
@sbidoul sbidoul removed the stale PR/Issue without recent activity, it'll be soon closed automatically. label Jan 21, 2024
@sbidoul sbidoul added the no stale Use this label to prevent the automated stale action from closing this PR/Issue. label Jan 21, 2024
rousseldenis
rousseldenis reviewed May 16, 2024
View reviewed changes
l10n_be_bpost_address_validation/wizards/bpost_address_validation.py


class BpostAddressValidationWizard(models.TransientModel):
_name = "bpost.address.validation.wizard"
Copy link
Contributor

Choose a reason for hiding this comment

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

@kouffsamuel Could you put a description attribute here ?

Odoo generates warnings :
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I fix it. Is it ok now ?

@lmignon lmignon mentioned this pull request Nov 7, 2024
Open
@lmignon
Copy link
Contributor

lmignon commented Nov 7, 2024

superseded by #228

@IT-Ideas
Copy link

IT-Ideas commented Nov 7, 2024 •
edited
Loading

@lmignon not sure if it uses the same api as the autocompletion module but if it is the case we should integrate the fix of #227

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@IT-Ideas IT-Ideas IT-Ideas left review comments

@rousseldenis rousseldenis rousseldenis left review comments

@lmignon lmignon lmignon requested changes

Assignees
No one assigned
Labels
no stale Use this label to prevent the automated stale action from closing this PR/Issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@kouffsamuel @lmignon @IT-Ideas @rousseldenis @sbidoul