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] l10n_in: raise warning for incorrect GST tax selection on overseas invoices #166109

Conversation

dhad-odoo
Copy link

@dhad-odoo dhad-odoo commented May 20, 2024

Before this PR:
When users selected incorrect Tax for exports where the 'place of supply' is 'Foregin Country(IN)' and the GST Treatment Type was set to Overseas, GSTR-1 did not compute the values. This caused difficulties for customers in tracing transactions that were not captured in GSTR-1.

After this PR:
Introduced a non-blocking warning banner on the account.move (invoice) when the GST Treatment is Overseas and the selected tax is not IGST. This helps users identify and correct tax selection issues, ensuring accurate capture of transactions in GSTR-1.

Task ID: 3921997

@robodoo
Copy link
Contributor

robodoo commented May 20, 2024

Pull request status dashboard.

@C3POdoo C3POdoo requested review from a team and dbkosky and removed request for a team May 20, 2024 10:30
@C3POdoo C3POdoo added the RD research & development, internal work label May 20, 2024
@dhad-odoo dhad-odoo force-pushed the master-l10n_in-imp-gst_tax_selection_on_overseas_invoice-dhad branch 2 times, most recently from 14d539d to add7932 Compare May 21, 2024 04:41
addons/l10n_in/models/account_invoice.py Outdated Show resolved Hide resolved
addons/l10n_in/views/account_invoice_views.xml Outdated Show resolved Hide resolved
addons/l10n_in/models/account_invoice.py Outdated Show resolved Hide resolved
@dhad-odoo dhad-odoo force-pushed the master-l10n_in-imp-gst_tax_selection_on_overseas_invoice-dhad branch 2 times, most recently from 990c0ce to 516d428 Compare May 24, 2024 05:46
@dhad-odoo
Copy link
Author

Thanks for the feedback, @dbkosky
I've updated the code.

Please review the changes.

Copy link
Contributor

@dbkosky dbkosky left a comment

Choose a reason for hiding this comment

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

passing this on for approval

Copy link
Contributor

@FlorianGilbert FlorianGilbert left a comment

Choose a reason for hiding this comment

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

Could you please add a test, thank you :)

@api.depends('invoice_line_ids.tax_ids', 'l10n_in_gst_treatment', 'l10n_in_state_id.code')
def _compute_l10n_in_warnings(self):
for move in self:
if move.country_code != 'IN':
Copy link
Contributor

Choose a reason for hiding this comment

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

Why country_code is not a dependency?

Comment on lines 162 to 163
not self.env.ref('l10n_in.tax_tag_igst') in tax.invoice_repartition_line_ids.tag_ids
for tax in move.invoice_line_ids.tax_ids.filtered(lambda tax: tax.country_code == 'IN')
Copy link
Contributor

Choose a reason for hiding this comment

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

Multiple things to say here:
Avoid doing a double loop here (your list comprehension and the filtered function).
Instead do something like this:

x = [
    a
    for a in my_list
    if a.country_code == 'IN'
]

After, avoid using .ref() in a loop, each time, you'll browse and call exists on the same new object. Save it in a variable before entering in your list comprehension.

Finally, instead of doing the not self.env.ref('l10n_in.tax_tag_igst') in tax.invoice_repartition_line_ids.tag_ids, inverse it for something like this:

self.env.ref('l10n_in.tax_tag_igst') not in tax.invoice_repartition_line_ids.tag_ids

Copy link
Author

Choose a reason for hiding this comment

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

I have updated the code according to the last-minute feedback from the product owner for improved functionality. I will incorporate the changes you mentioned and push the new code soon. @FlorianGilbert

Thank you

@dhad-odoo dhad-odoo force-pushed the master-l10n_in-imp-gst_tax_selection_on_overseas_invoice-dhad branch 2 times, most recently from ce43d8e to faa507b Compare June 5, 2024 10:25
…eas invoices

Before this commit:
When users selected incorrect Tax for exports where the 'place of supply' is
'Foregin Country(IN)' and the GST Treatment Type was set to Overseas, GSTR-1
did not compute the values. This caused difficulties for customers in
tracing transactions that were not captured in GSTR-1.

After this commit:
Introduced a non-blocking warning banner on the account.move (invoice) when
the GST Treatment is Overseas, place of supply is 'Foregin Country(IN)' and
the selected tax is not IGST. This helps users identify and correct tax
selection issues, ensuring accurate capture of transactions in GSTR-1.

Task ID - 3921997
@dhad-odoo dhad-odoo force-pushed the master-l10n_in-imp-gst_tax_selection_on_overseas_invoice-dhad branch from faa507b to 3c80afd Compare June 5, 2024 11:11
@dhad-odoo
Copy link
Author

@FlorianGilbert , I have added the test case and made the changes you mentioned.

@FlorianGilbert
Copy link
Contributor

According to @tsb-odoo comment on the task, we won't merge this PR. Feel free to open it again if PO change their mind.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RD research & development, internal work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants