-
-
Notifications
You must be signed in to change notification settings - Fork 328
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
[MIG] document_url port v10 #111
Conversation
Hey @difusionvisual, thank you for your Pull Request. It looks like some users haven't signed our Contributor License Agreement, yet.
Appreciation of efforts, |
Have you signed the CLA? |
This user is included in the "Entity CLA" "Serincloud S.L." few days ago. Un saludo,
antonio.canovas@ingenieriacloud.com
2016-11-15 18:28 GMT+01:00 Pedro M. Baeza notifications@github.com:
http://www.konery.es Muchas gracias. |
Functionality LGFM |
Use the new approval system (in the third tab) |
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.
Functionality LGFM
Thanks for this contribution, please check travis it fails |
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.
- Change 9.0 to 10.0 in https://github.com/OCA/knowledge/pull/111/files#diff-aea8538352c095a1b0f3280f36aca520R26
- Use new README template: https://raw.githubusercontent.com/OCA/maintainer-tools/master/template/module/README.rst
url = urlparse(form.url) | ||
if not url.scheme: | ||
url = urlparse('%s%s' % ('http://', form.url)) | ||
for active_id in context.get('active_ids', []): | ||
for active_id in self._context.get('active_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.
Use self.env.context
instead.
Fixed review |
document_url/static/src/xml/url.xml
Outdated
</li> | ||
</t> | ||
</t> | ||
</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.
Line at the end.
document_url/README.rst
Outdated
* Pedro M. Baeza <pedro.baeza@tecnativa.com | ||
* Pedro M. Baeza <pedro.baeza@tecnativa.com> | ||
* Nicolás Ramos <contacto@difusionvisual.com> | ||
* Antonio Cánovas <antonio.canovas@ingenieriacloud.com> |
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.
Non developers shouldn't go here as contributors.
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 happens a lot - Do you think a note in the contribution guidelines would be worthy?
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.
Well, sometimes a "designer" can be put here, because he/she has contributed in the design of the module, but I know the history of this module (I developed it initially for him), and there's no contribution except the financial one. The line here is thin, so it's difficult to reflect in a guideline...
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 have created a PR to improve the template to add the possibility of defining funders: OCA/maintainer-tools#241
document_url/wizard/document_url.py
Outdated
'res_id': active_id, | ||
'res_model': context['active_model'], | ||
'res_model': context.get('active_model') |
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.
Why you change this? Is correct as is.
Tried on runbot and working. |
@difusionvisual I got the following error on runbot: Can you have a look? |
document_url/static/src/js/url.js
Outdated
* Pedro M. Baeza <pedro.baeza@serviciosbaeza.com> | ||
* © 2016 ACSONE SA/NV (<http://acsone.eu>) | ||
* License AGPL-3.0 or later (http://www.gnu.org/licenses/agpl.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.
Don't remove copyrights
document_url/wizard/document_url.py
Outdated
if not context.get('active_model'): | ||
|
||
context = self.env.context | ||
if not context['active_model']: |
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.
Keep the original context.get
- the new form will error if the looked up key is not found in the context.
document_url/wizard/document_url.py
Outdated
url = urlparse(form.url) | ||
if not url.scheme: | ||
url = urlparse('%s%s' % ('http://', form.url)) | ||
for active_id in context.get('active_ids', []): | ||
for active_id in context['active_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.
Keep the context.get()
.
That's the cause of the error found by @sbidoul
attachment_obj.create(cr, uid, attachment, context=context) | ||
return {'type': 'ir.actions.act_close_wizard_and_reload_view'} | ||
attachment_obj.create(attachment) | ||
return 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.
Why is the return value now empty?
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 don't understand why this action repoen the wizard is it a "quick next encode"? I agree with @difusionvisual to replace this by return 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.
It was because in older versions of Odoo, it didn't refresh the view and you didn't see the new attachment number. I don't know if this happens also in v10.
@nicolasramos do you plan to finish this one? |
waiting merge of https://github.com/ingenieriacloud/knowledge/pull/2 with changes of @dreispt |
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 tried the module in my v10 and works fine.
Travis is still red |
For Travis only need minimal changes in two files. I have the Travis ok with only this changes: |
superseded by #192 |
No description provided.