-
-
Notifications
You must be signed in to change notification settings - Fork 693
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
[ADD] [15.0]account_invoice_date #1166
Conversation
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.
Code review and functional test.
LGTM!
Commit history is missing. Can you follow the steps described here https://github.com/OCA/maintainer-tools/wiki/Migration-to-version-15.0 ? |
Is it a migration or a new one? |
If this is a new module, can you please remove the word "migration" from the title here as well? thanks :) |
Hello @ikapasi-initos , I'm curious about why you this this module. Is this a UX button?
|
49dfc0d
to
d2ace09
Compare
Yes, it's a button which allows to choose the invoice date when confirming the invoice. This helps when the company uses "actual taxation" (instead of "imputed taxation") which is not supported by Odoo directly. |
Hello @dalonsod it would be great if you add reviews here. |
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 for the contribution!
Tested in runboat, AFAIK works. Code review, in general OCA conventions for new addons should be followed and pre-commit run -a
command seems to be missing, see comments.
@@ -0,0 +1,6 @@ | |||
==================== |
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.
This file should not be included, it'll be autogenerated later
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.
Hi, @dalonsod when I remove the " README.rst" file and then I've run the "pre-commit run -a" command faced an issue in the " README.rst" file. Please find the attached image.
@@ -0,0 +1,106 @@ | |||
# Translation of Odoo Server. |
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.
Please remove this, translations should be added using Weblate
@@ -0,0 +1,5 @@ | |||
# -*- coding: utf-8 -*- |
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.
These headers are actually unncessary in python files
|
||
|
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.
Remove extra blank lines.
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.
removed extra lines
</button> | ||
</field> | ||
</record> | ||
</odoo> |
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.
Missing empty line at file end
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.
An added empty line.
import logging | ||
|
||
from odoo import api, fields, models | ||
|
||
_logger = logging.getLogger(__name__) |
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.
Unused import and variable, and api
not used
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.
Removed unnecessary imports
_description = "Account Voucher Proforma Date" | ||
|
||
date = fields.Date( | ||
string="Rechnungsdatum", required=True, default=fields.Date.context_today |
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.
string
attribute in english
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.
Added the English string.
account_invoice_date/__manifest__.py
Outdated
# License AGPL-3.0 or later (http://www.gnu.org/licenses/agpl). | ||
{ | ||
"name": "Account Invoice Date", | ||
"description": """ |
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.
Use summary
instead of description
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.
Replaced description with a summary.
account_invoice_date/__manifest__.py
Outdated
""", | ||
"category": "Accounting", | ||
"version": "15.0.1.0.0", | ||
"license": "LGPL-3", |
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.
License not coherent with file header
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.
BTW, we don't have a strong preference. Does OCA prefer any particular license or use by default?
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.
@jans23 Both are accepted and it is the author choice. Tipically it is AGPL unless it is a base module that is meant to be used in many other places. I think this module will not be used as dependency for others so APGL is fine.
d2ace09
to
0ded821
Compare
Hi,@dalonsod I've implemented the feedback you can review it. |
Hi, @BT-anieto it would be great if you add reviews here |
account_invoice_date/README.rst
Outdated
==================== | ||
|
||
When an invoice is validated from Draft to Open automatically, the invoice date | ||
is set to current date.If an invoice is validated from Draft to Open manually, a window is displayed to query the invoice date from the user. |
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.
is set to current date.If an invoice is validated from Draft to Open manually, a window is displayed to query the invoice date from the user. | |
is set to current date. If an invoice is validated from Draft to Open manually, a window is displayed to query the invoice date from the user. |
Can you put this in the first comment of the PR? this is a new module. People should know what is this about without looking at the code. Thanks.
0ded821
to
1306823
Compare
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.
Functional review works fine
<group> | ||
<field | ||
name="date" | ||
string="Please enter invoice date (should correspond to payment date)" |
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.
string="Please enter invoice date (should correspond to payment date)" | |
string="Please enter invoice date" |
I would avoid the part between brackets, but it is not blocking.
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.
Hi, @AaronHForgeFlow I've implemented the changes.
8c77dae
to
42292df
Compare
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.
42292df
to
e08ad33
Compare
@AaronHForgeFlow Could you please review it? |
Hi, @max3903 Could you please review this PR? |
7bf57ea
to
0f2295b
Compare
Sure, I'll add the test case |
402da0b
to
9d7a5c8
Compare
@AaronHForgeFlow, I've added the test cases kindly review them. Thank you |
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.
Functional review
9d7a5c8
to
263794c
Compare
Hello @dalonsod, In this PR all the changes as per your feedback have been done, Kindly review it. |
I still see |
I don't understand. Why should the German translation be removed? Is there an alternative to the |
Well, for new modules adding |
This PR has the |
263794c
to
6fa1ec5
Compare
LGTM |
@AaronHForgeFlow This PR is approved and read-to-merge. What can we do to get it merged? |
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.
When an customer invoice or vendor bill is validated from Draft to Open automatically, the invoice/bill date is set to current date. If an invoice/bill is validated from Draft to Open manually, a window is displayed to query the invoice/bill date from the user.
The name of the module IMHO is incorrecto as indicate that you are adding fields date in invoices
@AaronHForgeFlow could you say a better name like account_invoice_date_update
or something similar?
Thank you all! 😄
We will happily change the module's name to whatever name you agree on. |
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. |
f4235e0
to
6fa1ec5
Compare
6fa1ec5
to
044d38e
Compare
* Modified account_invoice_date/models/account_move.py so that a name is always set * Changed the way name is set in order to work with tranlations
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. |
No description provided.