-
Notifications
You must be signed in to change notification settings - Fork 24.4k
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] hr_expense: usability improvements #35400
[IMP] hr_expense: usability improvements #35400
Conversation
4bc3daf
to
9ed3540
Compare
9ed3540
to
2366b3c
Compare
2366b3c
to
66fac0b
Compare
66fac0b
to
8439cc6
Compare
8439cc6
to
16b6eb7
Compare
16b6eb7
to
4f58d3a
Compare
4f58d3a
to
32ae0da
Compare
32ae0da
to
80cd2a2
Compare
80cd2a2
to
d56e555
Compare
d56e555
to
762f051
Compare
762f051
to
e851d8d
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.
Small comments. Could you adapt please ?
Thanks.
@@ -156,6 +156,42 @@ def _onchange_product_uom_id(self): | |||
if self.product_id and self.product_uom_id.category_id != self.product_id.uom_id.category_id: | |||
raise UserError(_('Selected Unit of Measure does not belong to the same category as the product Unit of Measure.')) | |||
|
|||
def create_expense_from_attachments(self, attachment_ids=[]): |
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.
1/ Did you consider the security ? You have a public method to create attachments from an arbitrary list of ids.
2/ You should never use a mutable object as a method parameter. See: https://www.tutorialdocs.com/article/9-worst-python-practices.html
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.
@tivisse
For point 1: I have put a condition, in that I have checked res_id and res_model values. If condition will be true then only 'create' will execute and records will be created.
For point 2: changes are done !
var qweb = core.qweb; | ||
var DocumentUploadMixin = { | ||
|
||
start: function () { |
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.
Could you adapt the code to:
- Match the odoo guidelines
- Match the linter requirements
Thanks.
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.
done !
}); | ||
|
||
odoo.define('hr_expense.expenses.tree', function (require) { | ||
"use strict"; |
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.
Should be split into 2 .js 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.
done !
e851d8d
to
e7a8907
Compare
e7a8907
to
ccba1f2
Compare
ccba1f2
to
041b3f6
Compare
For better user experience, there should be a 'UPLOAD' button on'hr.expense' through which user can directly upload a single or multiple documents and according to the number of documents, blank expense record with attached document should be generated. These records should be in 'draft' state. TaskID: 2040887
041b3f6
to
86dc991
Compare
@robodoo r+ |
For better user experience, there should be a 'UPLOAD' button on'hr.expense' through which user can directly upload a single or multiple documents and according to the number of documents, blank expense record with attached document should be generated. These records should be in 'draft' state. TaskID: 2040887 closes #35400 Signed-off-by: Yannick Tivisse (yti) <yti@odoo.com>
Merged at bf347e7, thanks! |
task - https://www.odoo.com/web?#id=2040887&action=333&active_id=131&model=project.task&view_type=form&menu_id=4720
pad - https://pad.odoo.com/p/r.a4335d455e929d48a75746eb6a62411d
For better user experience, there should be a 'UPLOAD' button on hr.expense
through which user can directly upload a single or multiple documents and
according to the number of documents, blank expense record with attached
document should be generated. These records should be in 'draft' state.
By this commit, this improvement has been achieved.