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

[ADD] Added the sales_report_product_image module #29

Closed
wants to merge 8 commits into from

Conversation

JayVora-SerpentCS
Copy link
Sponsor

No description provided.

#
##############################################################################

from . import models
Copy link
Member

Choose a reason for hiding this comment

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

Line at the end

@pedrobaeza
Copy link
Member

Thanks for proposing this useful module to OCA.

Please rename it to sale_order_product_image to follow naming conventions: area (sale) + object (order) + function (product_image).

And please also remove your logo and put OCA generic one or better if you provide an specific one.

@JayVora-SerpentCS
Copy link
Sponsor Author

All done!

@rafaelbn
Copy link
Member

rafaelbn commented Sep 8, 2015

Where is runbot? This repo is not integrated with runbot?

@JayVora-SerpentCS
Copy link
Sponsor Author

@pedrobaeza , Ready to merge!
Thanks.


{
'name': 'Product image for sale reports',
'author': 'Serpent Consulting Services Pvt. Ltd.,\
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

replace \ with '

@pedrobaeza
Copy link
Member

I have activated runbot for next time you make a commit to build a test instance. @JayVora-SerpentCS, please rename it to the name I have told you (sale_order_..., not sale_report_...). It has more sense, because it also adds the image at sale order form view.

@JayVora-SerpentCS
Copy link
Sponsor Author

Updated!

@pedrobaeza
Copy link
Member

So finally the web_tree_image is needed? I have added web dependency on runbot and restart it to allow to try the module. I'll tell you when finish


{
'name': 'Product image for sale reports',
'author': 'Serpent Consulting Services Pvt. Ltd.,\
Copy link
Member

Choose a reason for hiding this comment

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

Don't make this line split, use:

    'author': 'Serpent Consulting Services Pvt. Ltd., '
              'Odoo Community Association (OCA)'

@pedrobaeza
Copy link
Member

You should provide a README.rst with module description following OCA's template: https://raw.githubusercontent.com/OCA/maintainer-tools/master/template/module/README.rst

Functionally speaking, I have tried the module on runbot and it works great, but there's some things you can improve:

  • Instead of having 2 fields for the image printing, you can have only one with the options: no image, big size image, medium size image, small size image, so that the selection is more coherent, and make this field required, defaulting to no image.
  • The image on the tree appears very small and seems useless. Maybe you can put an option to make it a bit larger.

Thanks anyway for the module!

@rousseldenis
Copy link
Sponsor Contributor

@JayVora-SerpentCS Could we consider this as too old and close ?

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

6 participants