-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
[14.0] new module sale_order_note_template (Sale Orders Terms and conditions Templates) #1431
Conversation
sale_terms_template/README.rst
Outdated
|badge1| |badge2| |badge3| |badge4| |badge5| | ||
|
||
This module add terms and condition template and change existing terms and condition | ||
(`sale_order.note`) field type from `Text` to `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.
as the field is note
, I think the technical name of module should use note also. like sale_order_note_template
.
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.
Do you suggest to use a technical name sale_order_note_template
different from the module name Sale terms template
in order to let functional users to find the module in apps store and let technical guys understand which fields is 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.
My proposal is to use the same words as Odoo sale module, so :
- technical module name
sale_order_note_template
. - Friendly module name
Sale Orders Terms and conditions Templates
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.
ok, thanks let's do that :)
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've renamed the module as suggest !
11d8bd8
to
f294ff4
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.
end of the review.
some minor remarks. LGTM otherwise. Thanks.
(code review / no test).
("jinja", "Jinja"), | ||
# Figure out why while saving qweb instructions there are wiped out | ||
# wich makes qweb templating not usable (probably for security reasons) | ||
# ("qweb", "QWeb"), |
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 text could be maybe moved in a ROADMAP.rst file. Don't you think ?
https:/OCA/maintainer-tools/tree/master/template/module/readme
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've done few more test without success, I don't realy needs qweb support so as suggest I've added a line in roadmap and removed that field at the moment.
readonly=True, | ||
states={"draft": [("readonly", False)]}, | ||
) | ||
note = fields.Html(readonly=True, states={"draft": [("readonly", False)]}) |
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.
If I understand correctly. you change the type of the field note
.
you should maybe add a post_init_hook
and / or a uninstall_hook
to handle correctly the conversion (Text
<--> Html
). Otherwise, I guess if you uninstall the module, you'll have a lot of html anchors in all the note
column. Don't you think ? (or maybe there is a core feature in Odoo to handle such conversion, I must confess that I have not tested to install your module, add some text in the note
field, and then, uninstall the module.)
If it's not in the scope of your PR, you can make a mention in a ROADMAP.rst section.
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'll move that in roadmap at the moment !
thanks for review
f294ff4
to
dda084b
Compare
…nvert field properly
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 Review Looks Good to me.
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.
Tested Functionally
This PR has the |
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! LGTM
Its seems that PR is ready to merge. It would be great if someone get merge this PR . |
Can this be merged? |
@legalsylvain It would be great if you can merge this PR. |
Seems that PR is approved is it possible to merge it ? |
/ocabot merge nobump |
What a great day to merge this nice PR. Let's do it! |
Congratulations, your PR was merged at 900612d. Thanks a lot for contributing to OCA. ❤️ |
No description provided.