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] sale_commission: Use one2many_tags widget #78

Merged
merged 3 commits into from
Apr 2, 2016

Conversation

pedrobaeza
Copy link
Member

This won't work until OCA/web#319 is merged.

UPDATE: Merged

@pedrobaeza
Copy link
Member Author

cc @ivantodorovich @hbrunn

@ivantodorovich
Copy link
Contributor

Looks good! 👍

@pedrobaeza
Copy link
Member Author

This one is ready to be reviewed

@@ -14,6 +14,19 @@
</field>
</record>

<record id="invoice_line_agent_form" model="ir.ui.view">
<field name="name">account.invoice.line.agent.form</field>

Choose a reason for hiding this comment

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

"name" field are unnecessary in view definition.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, but I always put them, because an empty name seems very ugly to me.

Choose a reason for hiding this comment

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

It will not be empty in database.
If no name is defined, the name will be autogenerated with model name + type (form / search / ...)
here for exemple it will be "account.invoice.line.agent" + "form".
Exactly, what you wrote.

(but not a blocking point, it's just a cosmetic comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, I didn't know! Thanks for the info.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a really good info!
Maybe it should be on the guidelines?

@pedrobaeza pedrobaeza force-pushed the 8.0-sale_commission-one2many_tags branch from 1b3acab to 09dfdcd Compare March 11, 2016 11:13
@pedrobaeza
Copy link
Member Author

@legalsylvain, remove name attribute in views

@ivantodorovich
Copy link
Contributor

I'm reviewing it.. but I'm noticing a few things:

  1. Default agent & commission is not populating
  2. name_get is only defined for invoice.lines.agent but not for sale.order.line.agent

Number 2. is easy to fix, but number 1. seems to be something wrong with the new widget, don't you think? Or maybe is my database? Is it working fine for you?

@ssaid
Copy link

ssaid commented Mar 17, 2016

I've tested too and agree with @ivantodorovich, Default agent & commission is not populating. I'll try to review what is happening there.

@pedrobaeza
Copy link
Member Author

@hbrunn, are you aware of this issue with defaults with your widget?

@hbrunn
Copy link
Member

hbrunn commented Mar 18, 2016

now I am. And now it's also obvious: https://github.com/OCA/web/blob/8.0/web_widget_one2many_tags/static/src/js/web_widget_one2many_tags.js#L25 - I don't do any effort here. Simply calling self.build_context() as done in https://github.com/OCA/web/blob/8.0/web_widget_one2many_tags/static/src/js/web_widget_one2many_tags.js#L54 should be enough to fix it.

@hbrunn
Copy link
Member

hbrunn commented Mar 18, 2016

@ssaid @ivantodorovich @pedrobaeza this is fixed in OCA/web#330

@jjscarafia
Copy link

I've try it in last runbot build and still same error of defaults not poppulating. Or perhups I'm doing something wrong...

@hbrunn
Copy link
Member

hbrunn commented Mar 23, 2016

if the last runbot build was built before yesterday, the fix was not included. have somebody with appropriate permissions trigger a rebuild, then it should work

@pedrobaeza
Copy link
Member Author

@hbrunn @jjscarafia, I launched a new build if I remember well, and my first test was also unsucessful, but I have triggered again another build. Please try when finished.

@pedrobaeza
Copy link
Member Author

@hbrunn, definitively it's not working. You can try here in runbot setting for example "Agent 1" on customer "Agrolait", and then making a sales order for that customer. Adding a line, commission line for "Agent 1" should be automatically populated. The same applies for invoices.

@hbrunn
Copy link
Member

hbrunn commented Mar 24, 2016

@pedrobaeza @jjscarafia @ssaid @ivantodorovich now it should be better: OCA/web#333

@hbrunn
Copy link
Member

hbrunn commented Mar 24, 2016

this problem doesn't matter for my use case, so consider this my good open source deed for the week ;-)

@jjscarafia
Copy link

Thank you very much @hbrunn!
I have tested locally your PR OCA/web#333 with this PR and I have this error:

  1. Create a sale order
  2. Choose a partner with an agent configured
  3. Add a line, default value cames ok to "agents and commissions" but I've an error when saving:

Uncaught Error: QWeb2 - template['ListView.row']: Runtime Error: Error: QWeb2 - template['ListView.row']: Runtime Error: Error: Comando m2m desconocido 0
http://localhost:8069/web/js/web.assets_common/3c4ca44:1020

@hbrunn
Copy link
Member

hbrunn commented Mar 30, 2016

@jjscarafia please turn on debug mode and try again. Then you will get a better error message with a line number from the widget javascript, please post that

@jjscarafia
Copy link

Thanks @hbrunn, here it goes...

Odoo

Client Error
Uncaught Error: Unknown m2m command 0
http://localhost:9069/web/static/src/js/view_list.js:1114

@hbrunn
Copy link
Member

hbrunn commented Mar 30, 2016

hmmm, so it crashes in the standard-code, weird. And the same works when you use the standard-widget? are you a bit familiar with javascript debugging? If so, please post the backtrace at this point and go up to the place where set_value is being called. What is the content of value there?

@jjscarafia
Copy link

Sorry but I don't know almost anything about javascript.
I'm sending console log, haven't found any "set_value"
IMPORTANT: without your widget I've another error, then one I've reported here #82 (not same error as far as I can see)
Log (same as image attached):

instance.web.ListView.List.instance.web.Class.extend.render_cell @ view_list.js:1114
QWeb.render.render_cell @ view_list.js:1239
(anonymous function) @ VM351:54
QWeb2.tools.foreach @ qweb2.js:167
(anonymous function) @ VM351:42
QWeb2.Engine.QWeb2.tools.extend._render @ qweb2.js:393
QWeb2.Engine.QWeb2.tools.extend.render @ qweb2.js:385
instance.web.ListView.List.instance.web.Class.extend.render_record @ view_list.js:1232
record_callbacks.change @ view_list.js:979
Events.trigger @ view_list.js:1798
instance.web.Class.extend._onRecordEvent @ view_list.js:2050
Events.trigger @ view_list.js:1798
instance.web.Class.extend.set @ view_list.js:1837
(anonymous function) @ view_list.js:1100
fire @ jquery.js:974
self.fireWith @ jquery.js:1084
self.fire @ jquery.js:1091
fire @ jquery.js:974
self.fireWith @ jquery.js:1084
(anonymous function) @ jquery.js:1136
fire @ jquery.js:974
self.fireWith @ jquery.js:1084
(anonymous function) @ jquery.js:1136
fire @ jquery.js:974
self.fireWith @ jquery.js:1084
done @ jquery.js:7803
callback @ jquery.js:8518

seleccion_027

@hbrunn
Copy link
Member

hbrunn commented Apr 1, 2016

@jjscarafia it should be fixed now. I also proposed to @pedrobaeza's branch pedrobaeza#1 to make it a bit more beautiful

…isplay_name

[IMP] give a display_name field to one2many_tags
@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 91.0% when pulling 92543e7 on pedrobaeza:8.0-sale_commission-one2many_tags into d96b745 on OCA:8.0.

@jjscarafia
Copy link

Thanks @hbrunn !! It's working perfectly now! Youn can edit the record by clicking on it!! Great work
👍

@hbrunn
Copy link
Member

hbrunn commented Apr 2, 2016

@jjscarafia good to hear! could you thumb up OCA/web#333 then too in order to get it merged fast?

@jjscarafia
Copy link

done, thanks again!

El sáb., 2 abr. 2016 a las 11:05, Holger Brunn (notifications@github.com)
escribió:

@jjscarafia https://github.com/jjscarafia good to hear! could you thumb
up OCA/web#333 OCA/web#333 then too in order to
get it merged fast?


You are receiving this because you were mentioned.

Reply to this email directly or view it on GitHub
#78 (comment)

@pedrobaeza
Copy link
Member Author

All is working now, so I merge.

@pedrobaeza pedrobaeza merged commit 07b122d into OCA:8.0 Apr 2, 2016
@pedrobaeza pedrobaeza deleted the 8.0-sale_commission-one2many_tags branch April 2, 2016 17:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants