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

[11.0][MIG] sale_triple_discount #627

Closed
wants to merge 7 commits into from

Conversation

xavierjimenez
Copy link
Contributor

@xavierjimenez xavierjimenez commented Apr 5, 2018

Migration of sale_triple_discount module to version 11.0

Depends on:

@xavierjimenez xavierjimenez changed the title 11.0 mig sale_triple_discount [11.0][MIG] sale_triple_discount Apr 5, 2018
@pedrobaeza pedrobaeza added this to the 11.0 milestone Apr 5, 2018
@pedrobaeza
Copy link
Member

You have added a .DS_store file

Copy link

@misern2 misern2 left a comment

Choose a reason for hiding this comment

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

You should update the security groups on the account_invoice views for the new fields.

Check core code for the discount field:

https://github.com/odoo/odoo/blob/00ef138ce038d4f8e5a9b7ad2f23ab483e282ebb/addons/sale/views/sale_views.xml#L718-L738

@rafaelbn
Copy link
Member

Thanks @xavierjimenez ! Please rebase as OCA/account-invoicing#370 has been merge and then we could test this one! 😄

@pedrobaeza
Copy link
Member

No need to rebase, as it's in another repository. Just to restart CIs.

@xavierjimenez
Copy link
Contributor Author

I'm not sure how to do that, do I have to open a new PR?

@pedrobaeza
Copy link
Member

I have restarted both.

@rafaelbn
Copy link
Member

There are missing dependencies

2018-04-18 16:06:05,315 5431 WARNING openerp_test odoo.modules.loading: Transient module states were reset
2018-04-18 16:06:05,315 5431 ERROR openerp_test odoo.modules.registry: Failed to load registry
Traceback (most recent call last):
File "/home/travis/OCB-11.0/odoo/modules/registry.py", line 85, in new
odoo.modules.load_modules(registry._db, force_demo, status, update_module)
File "/home/travis/OCB-11.0/odoo/modules/loading.py", line 308, in load_modules
modules.button_install()
File "<decorator-gen-39>", line 2, in button_install
File "/home/travis/OCB-11.0/odoo/addons/base/module/module.py", line 71, in check_and_log
return method(self, *args, **kwargs)
File "/home/travis/OCB-11.0/odoo/addons/base/module/module.py", line 397, in button_install
modules._state_update('to install', ['uninstalled'])
File "/home/travis/OCB-11.0/odoo/addons/base/module/module.py", line 362, in _state_update
raise UserError(_("You try to install module '%s' that depends on module '%s'.\nBut the latter module is not available in your system.") % (module.name, dep.name,))
odoo.exceptions.UserError: ("You try to install module 'sale_triple_discount' that depends on module 'account_invoice_triple_discount'.\nBut the latter module is not available in your system.", '')
2018-04-18 16:06:05,316 5431 CRITICAL openerp_test odoo.service.server: Failed to initialize database `openerp_test`.
Traceback (most recent call last):
File "/home/travis/OCB-11.0/odoo/service/server.py", line 925, in preload_registries
registry = Registry.new(dbname, update_module=update_module)
File "/home/travis/OCB-11.0/odoo/modules/registry.py", line 85, in new
odoo.modules.load_modules(registry._db, force_demo, status, update_module)
File "/home/travis/OCB-11.0/odoo/modules/loading.py", line 308, in load_modules
modules.button_install()
File "<decorator-gen-39>", line 2, in button_install
File "/home/travis/OCB-11.0/odoo/addons/base/module/module.py", line 71, in check_and_log
return method(self, *args, **kwargs)
File "/home/travis/OCB-11.0/odoo/addons/base/module/module.py", line 397, in button_install
modules._state_update('to install', ['uninstalled'])
File "/home/travis/OCB-11.0/odoo/addons/base/module/module.py", line 362, in _state_update
raise UserError(_("You try to install module '%s' that depends on module '%s'.\nBut the latter module is not available in your system.") % (module.name, dep.name,))
odoo.exceptions.UserError: ("You try to install module 'sale_triple_discount' that depends on module 'account_invoice_triple_discount'.\nBut the latter module is not available in your system.", '')

@murtuzasaleh
Copy link
Member

@xavierjimenez I am starting working on it.
cc @pedrobaeza

@xavierjimenez
Copy link
Contributor Author

@murtuzasaleh I think the module is working fine, there's a dependency issue with another module (account_invoice_triple_discount) but that module is already merged so I'm not sure how I can solve that.

@pedrobaeza
Copy link
Member

Is the repository put in oca_dependencies.txt in root folder?

@pedrobaeza
Copy link
Member

@xavierjimenez can you check what I have said?

@xavierjimenez
Copy link
Contributor Author

I just checked it out and it's not, can I add it?

@pedrobaeza
Copy link
Member

Yes, of course. That's the idea when you add a new module (migrated or totally new) that depends on modules on other repositories. Doing so, you will get the module available and Travis/runbot will be OK.

@pedrobaeza
Copy link
Member

All green! @murtuzasaleh @JayVora-SerpentCS can you please review this one now that the problem has gone?


.. image:: https://odoo-community.org/website/image/ir.attachment/5784_f2813bd/datas
:alt: Try me on Runbot
:target: https://runbot.odoo-community.org/runbot/167/10.0
Copy link
Contributor

Choose a reason for hiding this comment

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

11.0 plz

'ADHOC SA, '
'Agile Business Group, '
'Tecnativa, '
'Odoo Community Association (OCA)',
Copy link
Contributor

Choose a reason for hiding this comment

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

Authorname should be added when you added a feature my friend. If not, add your name to contributors in readme.

'Agile Business Group, '
'Tecnativa, '
'Odoo Community Association (OCA)',
'website': 'https://odoo-community.org',
Copy link
Contributor

Choose a reason for hiding this comment

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

'license': 'AGPL-3',
'summary': 'Manage triple discount on sale order lines',
'depends': [
'sale',
Copy link
Member

@murtuzasaleh murtuzasaleh Jul 9, 2018

Choose a reason for hiding this comment

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

IMO it should be sale_management instead of a sale module.

@xavierjimenez
Copy link
Contributor Author

Changes done! I'll make the squash with the previous commits when it's ready to merge.

@rafaelbn
Copy link
Member

rafaelbn commented Aug 1, 2018

Hi @murtuzasaleh @JayVora-SerpentCS @misern2 this PR is all green and attended comment.

@xavierjimenez just take a look in:

Thanks a lot for your 🍏 work! 👍

cc @chienandalu

@rafaelbn
Copy link
Member

rafaelbn commented Aug 1, 2018

Hi @xavierjimenez I've tested it and my doubt is about report. Does it have any sense for you?

Take a look:

ex1


ex2

Copy link
Member

@rafaelbn rafaelbn left a comment

Choose a reason for hiding this comment

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

I'm not blocking but IMHO report (PDF of SO) must be improved

@xavierjimenez
Copy link
Contributor Author

Hi @rafaelbn, thanks for the review!

I agree, the report needs a change so I'll try to add the others discounts on the report if they are set.

@pedrobaeza
Copy link
Member

@xavierjimenez I have just discovered the field price_reduce: https://github.com/odoo/odoo/blob/11.0/addons/sale/models/sale.py#L783, that contains the price with the discount. I'm not sure if this field is used everywhere, but maybe changing the computing of it can be enough.

Can you please try?

@rafaelbn
Copy link
Member

Hello @xavierjimenez @misern2 !

Are you using this module in production enviroment?

Take care for fixes in v10 that apply in this PR

We could try to push this PR reviewing fixes of v10, rebasing this PR and checking again.

Thanks

@rousseldenis
Copy link
Contributor

Closing this as very old and no answer from author. Please feel free to reopen it if needed.

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.

10 participants