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

[IMP] finanace: avatax api integration rewrite #8997

Closed
wants to merge 1 commit into from

Conversation

@tiku-odoo tiku-odoo self-assigned this Apr 25, 2024
@robodoo
Copy link
Collaborator

robodoo commented Apr 25, 2024

@tiku-odoo tiku-odoo force-pushed the 15.0-finance-accounting-avatax-rewrite-tiku branch 3 times, most recently from 41f6de6 to fe33ce6 Compare April 29, 2024 21:38
@tiku-odoo tiku-odoo marked this pull request as ready for review April 29, 2024 21:38
@C3POdoo C3POdoo requested a review from a team April 29, 2024 21:40
@tiku-odoo tiku-odoo force-pushed the 15.0-finance-accounting-avatax-rewrite-tiku branch from fe33ce6 to 9c82e74 Compare May 1, 2024 20:40
@tiku-odoo tiku-odoo force-pushed the 15.0-finance-accounting-avatax-rewrite-tiku branch from 9c82e74 to 1138226 Compare May 1, 2024 20:48
@tiku-odoo
Copy link
Contributor Author

@rsh-odoo @chiaraprattico

When each of you has a moment, can you review the Avatax rewrite?

Both of you were involved in the Avatax script edit for Jose's eLearning video, so I decided to include you on this review.

I am creating a separate doc for the Avalara Portal and Tax Calculation process.

Thanks for your time with this. Please make comments.

👍 Tim

@chiaraprattico
Copy link
Contributor

Hi @tiku-odoo, thank you. I'll leave this one as I don't know the subject well enough. You can tag localization review once you need our review on a doc guideline pov.

@tiku-odoo tiku-odoo requested a review from Felicious May 2, 2024 13:53
@tiku-odoo tiku-odoo force-pushed the 15.0-finance-accounting-avatax-rewrite-tiku branch from 1138226 to f4ad233 Compare May 2, 2024 16:01
@tiku-odoo
Copy link
Contributor Author

@Felicious

This doc is ready for your review when you have a moment.

Thanks,
Tim 👍

@tiku-odoo tiku-odoo requested review from dade-odoo, toaa-odoo and a team and removed request for dade-odoo, chiaraprattico and toaa-odoo May 2, 2024 21:27
@tiku-odoo
Copy link
Contributor Author

@odoo/localizations-doc-review

Here is the revised AvaTax integration doc for your review when you have a moment.

Thanks!

Tim

Copy link
Contributor

@Felicious Felicious left a comment

Choose a reason for hiding this comment

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

Hello @tiku-odoo !

I just wanted to drop a quick note to say fantastic work on putting together such clear and concise instructions for configuring all these different components. Your instructions were spot-on and worked perfectly during my runbot testing. It's evident that you put a lot of care into your testing and research, and I really appreciate the effort you've put into gathering and documenting all this information so quickly!

I did have a few minor suggestions for places where a bit more detail could be helpful for those who might not be familiar with every aspect of the AvaTax setup. And I also have a couple of optional rewording suggestions, just to consider.

Overall, though, I think you've done an incredible job with this document, and I'm excited to see it move forward to the next stage of review! Keep up the great work!

content/applications/finance/accounting/taxes/avatax.rst Outdated Show resolved Hide resolved
content/applications/finance/accounting/taxes/avatax.rst Outdated Show resolved Hide resolved
content/applications/finance/accounting/taxes/avatax.rst Outdated Show resolved Hide resolved
content/applications/finance/accounting/taxes/avatax.rst Outdated Show resolved Hide resolved
content/applications/finance/accounting/taxes/avatax.rst Outdated Show resolved Hide resolved
content/applications/finance/accounting/taxes/avatax.rst Outdated Show resolved Hide resolved
content/applications/finance/accounting/taxes/avatax.rst Outdated Show resolved Hide resolved
content/applications/finance/accounting/taxes/avatax.rst Outdated Show resolved Hide resolved
content/applications/finance/accounting/taxes/avatax.rst Outdated Show resolved Hide resolved
content/applications/finance/accounting/taxes/avatax.rst Outdated Show resolved Hide resolved
@tiku-odoo tiku-odoo force-pushed the 15.0-finance-accounting-avatax-rewrite-tiku branch 2 times, most recently from fc3b77c to 06484eb Compare May 3, 2024 18:25
@tiku-odoo tiku-odoo requested a review from a team May 3, 2024 18:26
@tiku-odoo
Copy link
Contributor Author

@ksc-odoo

This doc is ready for your review when you have a moment.

Thanks!

Tim

@tiku-odoo
Copy link
Contributor Author

@samueljlieber

This doc is ready for your review when you have a moment.

Thanks!
Tim

Copy link
Contributor

@samueljlieber samueljlieber left a comment

Choose a reason for hiding this comment

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

Hi @tiku-odoo, this is an awesome PR and a huge content-add to documentation! Overall I dont have many changes, just a small handful of suggestions and corrections, please see below. I think it would be best for @ksc-odoo to do a full content review after this as well, just to be thorough.

Great work on this @tiku-odoo, looking forward to seeing this one out in the wild 💪

content/applications/finance/accounting/taxes/avatax.rst Outdated Show resolved Hide resolved
content/applications/finance/accounting/taxes/avatax.rst Outdated Show resolved Hide resolved
content/applications/finance/accounting/taxes/avatax.rst Outdated Show resolved Hide resolved
content/applications/finance/accounting/taxes/avatax.rst Outdated Show resolved Hide resolved
content/applications/finance/accounting/taxes/avatax.rst Outdated Show resolved Hide resolved
content/applications/finance/accounting/taxes/avatax.rst Outdated Show resolved Hide resolved
content/applications/finance/accounting/taxes/avatax.rst Outdated Show resolved Hide resolved
content/applications/finance/accounting/taxes/avatax.rst Outdated Show resolved Hide resolved
content/applications/finance/accounting/taxes/avatax.rst Outdated Show resolved Hide resolved
content/applications/finance/accounting/taxes/avatax.rst Outdated Show resolved Hide resolved
@tiku-odoo tiku-odoo force-pushed the 15.0-finance-accounting-avatax-rewrite-tiku branch from 06484eb to ceaaf6b Compare May 6, 2024 14:46
@tiku-odoo tiku-odoo requested a review from a team May 6, 2024 14:47
@tiku-odoo
Copy link
Contributor Author

@ksc-odoo

This doc is ready for your review when you have a moment.

Thanks,
Tim 👍

@tiku-odoo
Copy link
Contributor Author

@odoo/localizations-doc-review

This PR is ready for your review when you have a moment. Thanks 👍

Tim

Copy link
Contributor

@chiaraprattico chiaraprattico left a comment

Choose a reason for hiding this comment

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

Hi @tiku-odoo great one, thank you!
Let me know if my comments are clear. I noticed a conflict too in the checks.
Same as for the other PR I imagine once forward ported you will adapt the screenshots to newer versions.

@tiku-odoo tiku-odoo force-pushed the 15.0-finance-accounting-avatax-rewrite-tiku branch from ceaaf6b to adc4cae Compare May 8, 2024 13:47
@tiku-odoo tiku-odoo changed the title [IMP] accounting: avatax api integration rewrite [IMP] finanace: avatax api integration rewrite May 8, 2024
@tiku-odoo tiku-odoo force-pushed the 15.0-finance-accounting-avatax-rewrite-tiku branch from adc4cae to 2b13950 Compare May 8, 2024 15:12
Copy link
Contributor

@ksc-odoo ksc-odoo left a comment

Choose a reason for hiding this comment

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

@tiku-odoo -- just finished my Final Review on this PR. Nice job! Very detailed and in-depth. I did leave a decent amount of comments, but they are all very minor/easy to deal with -- so I am approving now. Once you implement all the necessary changes, feel free to tag this for Tech Review. Thanks! 👍

content/applications/finance/accounting/taxes/avatax.rst Outdated Show resolved Hide resolved
content/applications/finance/accounting/taxes/avatax.rst Outdated Show resolved Hide resolved
content/applications/finance/accounting/taxes/avatax.rst Outdated Show resolved Hide resolved
content/applications/finance/accounting/taxes/avatax.rst Outdated Show resolved Hide resolved
content/applications/finance/accounting/taxes/avatax.rst Outdated Show resolved Hide resolved
content/applications/finance/accounting/taxes/avatax.rst Outdated Show resolved Hide resolved
content/applications/finance/accounting/taxes/avatax.rst Outdated Show resolved Hide resolved
content/applications/finance/accounting/taxes/avatax.rst Outdated Show resolved Hide resolved
content/applications/finance/accounting/taxes/avatax.rst Outdated Show resolved Hide resolved
content/applications/finance/accounting/taxes/avatax.rst Outdated Show resolved Hide resolved
@tiku-odoo tiku-odoo force-pushed the 15.0-finance-accounting-avatax-rewrite-tiku branch from 2b13950 to 5450690 Compare May 9, 2024 17:34
@tiku-odoo tiku-odoo requested a review from a team May 9, 2024 17:35
@tiku-odoo
Copy link
Contributor Author

@samueljlieber

This doc has been reviewed by KC. You've already reviewed it, but I thought you might want to take another look to merge.

Thanks,
👍
Tim

Copy link
Contributor

@samueljlieber samueljlieber left a comment

Choose a reason for hiding this comment

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

Hi @tiku-odoo, thanks for tagging me for one more review. This PR looks great to me! I flagged a couple quick fixes, please see below. Other than these I think this PR is good to go, so approving and delegating merge to you. Thanks for your hard work on this, and all the reviewers as well! 🚀
.....
@robodoo delegate=tiku-odoo

content/applications/finance/accounting/taxes/avatax.rst Outdated Show resolved Hide resolved
content/applications/finance/accounting/taxes/avatax.rst Outdated Show resolved Hide resolved
content/applications/finance/accounting/taxes/avatax.rst Outdated Show resolved Hide resolved
content/applications/finance/accounting/taxes/avatax.rst Outdated Show resolved Hide resolved
@tiku-odoo tiku-odoo force-pushed the 15.0-finance-accounting-avatax-rewrite-tiku branch from 5450690 to 9b83359 Compare May 9, 2024 19:10
@tiku-odoo
Copy link
Contributor Author

@robodoo r+

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.

None yet

6 participants