-
-
Notifications
You must be signed in to change notification settings - Fork 62
[16.0][ADD] account_avatax_exemption_sign_oca #79
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
base: 16.0
Are you sure you want to change the base?
[16.0][ADD] account_avatax_exemption_sign_oca #79
Conversation
|
@dnplkndll I will work on tests |
fa73a89 to
bdee966
Compare
AlexPForgeFlow
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Module account_avatax_exemption should be removed from MR as does not belong to this repo.
| @api.depends("state") | ||
| def _cancel_sign_request_id(self): | ||
| for rec in self: | ||
| if rec.state == "cancel": | ||
| rec.sign_oca_request_id.cancel() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| @api.depends("state") | |
| def _cancel_sign_request_id(self): | |
| for rec in self: | |
| if rec.state == "cancel": | |
| rec.sign_oca_request_id.cancel() | |
| def write(self, vals): | |
| if vals.get("state") == "cancel": | |
| self.sign_oca_request_id.cancel() | |
| res = super().write(vals) |
@api.depends only would be triggered to compute fields (don't apply in this field), so I suggest changing the logic to handle cases on write method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, I added this module in a separate PR.
here I added it only because we wanted to test sign exemption and the module for exemption is not merged in 16.0 branch.
we tried to add it from the PR in test requirements as dependency but didn't succeed.
we can resume discussion about exemption module in its open PR for 16.0 branch.
bdee966 to
56c46fe
Compare
56c46fe to
913cca7
Compare
9bbf926 to
16eeba1
Compare
|
There were some tests needed update after we modified the functionality under write method. |
AlexPForgeFlow
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 👍
5ca1344 to
948d6e8
Compare
|
Observe in the exemption view I display the exemption name, and due to: I fix an issue with name as there was a treatment with a virtual name, but now now is really exited in the database and editable thanks to the constrain I introduce there. |
|
By the way, jut to remind myself and other devs. onchange is concerning view changes ONLY but constrains are not bound to views. I didn't commit the same of the |
|
what do you think of this commit? |
|
I still have one more suggestion to show up the sign_request field in the view if it is there same as what we did in the dms exemption connector so we can go between both views easily and quickily. |
|
@kobros-tech regarding this commit 948d6e8 . |
all right here is the log of the issue in detail: WRITE called on exemption res.partner.exemption(51,) with vals: {'name': 'Demo Type - 123456 - YourCompany, Mitchell Admin'}
|
|
@AlexPForgeFlow |
|
@AlexPForgeFlow That is the side effect of using onchange method, I don't complain about api decorators I use in backend but in this case issue happened. |
|
@kobros-tech When you say the line is deleted, are you referring the exemption lines from a partner? |
0d08770 to
a01a068
Compare
|
This commit is essential for connecting survey, sign, and exemption together: |
you only need to change my methods with on change methods, but you only see this if exemption is created in the backend like when you validate a sign request for exemption. so using this PR and sign oca you need to interchange current methods and will see an error warning preventing you from validating the sign request. |
a01a068 to
70ff4a3
Compare
|
@pedrobaeza what is the process from here on an ADD? |
pedrobaeza
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm struggling with these things:
- The name of the module: it's something more on the sign part, or the Avatax one? That would determine the starting part of the name
- Being any side or the other for the name, Avatax is something very specific of.. USA? Being here in this general repo seems to not be the correct place.
- The README is very short explaining concepts and the purpose of the module. What is Avatax exemption? How is this related to sign?...
|
name: I am indifferent and can ask on the other project side it appropriate. @dreispt readme |
|
Thanks for the explanation, so, of I understood correctly, this is not something related specifically to Avatax, but tax exemption in general. Or the module contains something tied to the Avatax platform? |
|
essentially the purpose if to add the document to support the customer exemption. but that is a good point are the exemptions used by other sales tax systems? I am not sure as to my knowledge we developed this as the first customer sales tax exemption management tool. We should probably be made general purpose to support any sales tax system. However we were not able to use the taxcloud that was the official system on 13.0 as it did not even support customer sales tax exmeptions fort reasons alike agricultural use. I am not certain if that has evened. Do you know of any other systems to review and support in addition to the avatax api? |
|
@pedrobaeza oh as to where to put it? just following the maintenance, project convention where is seemed like the sign relates module extensions are in sign. same issue on the PDF supporting document in dms OCA/dms#395 Both depend on an exemption doc to support with a signed doc. |
|
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. |
|
Did you ask to add this module to another project, which one in your opinion? |
|
AFAIK, l10n-usa is hosting the |
70ff4a3 to
94a97cd
Compare
|
I'm now seeing that the dependent module is in https://github.com/OCA/account-fiscal-rule/tree/16.0/account_avatax_exemption, so there should go this one. |
|
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. |
account_avatax_exemption_sign_oca
proposing a new module to streamline the sale tax pdf exemption sign and storage using the sign_oca app and the account_avatax_exemption
we created and used this internal for several years now. Will be adding the account_avatax_exemption_dms next for paper based scan. not in digital signature and then hope to next gave a portal feature allowing the customer to upload or sign request a renewal requirement on a given state sales tax exemption
I wanted to propose the inclusion in oca with very general use case and can then complete test and migrate.
@JordiBForgeFlow @dreispt @atchuthan