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

[14.0][ADD] ebill_postfinance #675

Merged
merged 2 commits into from
Dec 6, 2023
Merged

Conversation

TDu
Copy link
Member

@TDu TDu commented Sep 28, 2022

No description provided.

ebill_postfinance/models/ebill_postfinance_service.py Outdated Show resolved Hide resolved
return res

def get_process_protocol_list(self, archive_data=False):
# Is this the processing result of an invoice ?
Copy link
Member

Choose a reason for hiding this comment

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

Should it translate as a TODO ?

Copy link
Member Author

@TDu TDu Oct 28, 2022

Choose a reason for hiding this comment

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

Questioning the use of this call to the api.
The situation at the moment is, all possible call to the api are implemented, so they can be used and tested.
But they are not all used yet by the module for real.
There is the roadmap file that contains some more information.

service = self._get_service()
res = service.get_registration_protocol_list(archive_data)
for registration_protocol in res or {}:
self.get_registration_protocol(registration_protocol.CreateDate)
Copy link
Member

Choose a reason for hiding this comment

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

the value returned from this function is not used

Copy link
Member Author

Choose a reason for hiding this comment

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

This call is related to the contract subscription update through the web service. And not yet implemented, actually when I tested it I could never get any data.
I learn today that the subscription update through the web service is only implemented for b2ebanking or b2c.

help="Timeout for each HTTP (GET, POST) request in seconds.",
)

def _get_service(self):
Copy link
Member

Choose a reason for hiding this comment

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

Does it create a new session each time, can't we reuse a session by storing the current service?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a very good point. And yes I believe there is some improvement to be done on this...

Copy link

Choose a reason for hiding this comment

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

Maybe decorating the method with @ormcache would work?

@TDu
Copy link
Member Author

TDu commented Oct 18, 2022

@yvaucher Thank you for the review, I will attend them.
For now I extracted the stock related code into ebill_postfinance_stock see last commit 17b06e2

@TDu
Copy link
Member Author

TDu commented Oct 28, 2022

Previous fixup fixes the number of references included for each invoice line in the xml payload.
Adds a payload size in the data, can be useful, knowing that the web service will refuse more than 5MB.

self.transaction_id = "-".join(
[
fields.Datetime.now().strftime("%y%m%d%H%M%S"),
self.invoice_id.name.replace("/", "").replace("_", ""),

Choose a reason for hiding this comment

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

I would use slugify to make sure there's no weird char.

@simahawk
Copy link

@TDu what's the overall state here?

@TDu
Copy link
Member Author

TDu commented Jul 18, 2023

I squashed all previous commit and then the last fixup adds the credit note to be sent to the server.

@TDu TDu force-pushed the ebillpostfinance branch 2 times, most recently from 424485b to 3a18b6f Compare July 19, 2023 06:38
@sebalix
Copy link

sebalix commented Oct 6, 2023

@TDu so we are good to merge this?

@ecino
Copy link

ecino commented Nov 30, 2023

Maybe you could increase the coverage of the tests? Also you can force-push to retrigger the runboat. Is this ready to be merged otherwise?

@TDu TDu force-pushed the ebillpostfinance branch 3 times, most recently from 39a2e84 to d9e5d2c Compare December 1, 2023 15:15
@TDu
Copy link
Member Author

TDu commented Dec 1, 2023

@sebalix The required Python library needed to be on Pypi. This is done.
@ecino More tests would be nice, but I will improve them when migrating to 16.0
@simahawk IMO this is ready for merge
@yvaucher Could you update your review

Copy link

@ecino ecino left a comment

Choose a reason for hiding this comment

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

Should you also add server-env and edi in file oca_requirements.txt ?

ebill_postfinance/models/account_move.py Outdated Show resolved Hide resolved
TDu and others added 2 commits December 6, 2023 08:47
Remove the dependency on stock in the `ebill_postfinance` module.
And add a new module `ebill_postfinance_stock` that will integrate
in the xml invoice informations related to the deliveries.

This is for Odoo implementation that sell only services and have no
stock to manage. And do not use the `stock` module.
@TDu
Copy link
Member Author

TDu commented Dec 6, 2023

Should you also add server-env and edi in file oca_requirements.txt ?

With the introduction of the use of PyPi to find depending modules, this is not needed anymore, as far as I know, in fact Travis runs without errors.
Thanks for your review.

Copy link
Member

@yvaucher yvaucher left a comment

Choose a reason for hiding this comment

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

My remarks for improvements can be addressed later, better to have a version we can migrate to other version than none.

@yvaucher
Copy link
Member

yvaucher commented Dec 6, 2023

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 14.0-ocabot-merge-pr-675-by-yvaucher-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit e155cb8 into OCA:14.0 Dec 6, 2023
3 of 5 checks passed
@OCA-git-bot
Copy link
Contributor

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

8 participants