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] 8.0 - report_xlsx #50

Merged
merged 2 commits into from
May 3, 2016
Merged

[ADD] 8.0 - report_xlsx #50

merged 2 commits into from
May 3, 2016

Conversation

sebalix
Copy link
Contributor

@sebalix sebalix commented Apr 12, 2016

This is a backport of #38 from @adrienpeiffer (migrated with the instructions found here: https://github.com/OCA/maintainer-tools/wiki/Migration-to-version-9.0#if-the-module-doesnt-exist-in-the-90-branch).
It fixes #49 and is needed to solve OCA/account-financial-reporting#8

@coveralls
Copy link

Coverage Status

Coverage increased (+0.07%) to 30.318% when pulling 91bb68b on osiell:8.0-report_xlsx into 364e764 on OCA:8.0.

@sebalix
Copy link
Contributor Author

sebalix commented Apr 26, 2016

Runbot is green, and ready for "review" (it is the same code that the v9 version).


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

Choose a reason for hiding this comment

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

Change this to 8.0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Question: where do we get this ID 143? I was looking for that in https://github.com/OCA/maintainer-tools/blob/master/CONTRIBUTING.md but didn't find this information.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you, I missed that.

@pedrobaeza
Copy link
Member

You can put the icon used in XLSX files for the module, as the report_xls do. And please squash your commits in one, as they are minimal.

@sebalix
Copy link
Contributor Author

sebalix commented Apr 26, 2016

@sebalix
Copy link
Contributor Author

sebalix commented Apr 26, 2016

Done.

@coveralls
Copy link

coveralls commented Apr 26, 2016

Coverage Status

Coverage increased (+0.8%) to 30.318% when pulling fe69f0f on osiell:8.0-report_xlsx into c368bad on OCA:8.0.

@pedrobaeza
Copy link
Member

@pedrobaeza you mean the same icon used by report_xls? https://github.com/OCA/reporting-engine/blob/9.0/report_xls/static/description/icon.png

I suppose there's a new one corresponding to last Excel versions that fits with XLSX format, isn't it? Or maybe an special icon not for Excel, but for the mimetype XLSX (like this search: https://www.google.es/search?q=xlsx+icon&safe=off&client=ubuntu&hs=bDY&channel=fs&source=lnms&tbm=isch&sa=X&ved=0ahUKEwjFv-W6qq3MAhViF8AKHQKoBbIQ_AUIBygB&biw=1700&bih=901)

@sebalix
Copy link
Contributor Author

sebalix commented Apr 27, 2016

Yep I already search that kind of icon for the XLSX format some days ago but did'nt find one with a correct license. I will take a look again.

@sebalix
Copy link
Contributor Author

sebalix commented Apr 27, 2016

Well, just passed half an hour to find out an icon compatible with the AGPL license, without any success. For now I just copied the one from report_xls, but even this one I'm not sure if it is really free.

@pedrobaeza
Copy link
Member

The icon is not ruled under AGPL. The possible open license to apply is Creative Commons, but in this case, I think we can use it without problems under the fair usage policy: http://libguides.mit.edu/usingimages

@sebalix
Copy link
Contributor Author

sebalix commented Apr 27, 2016

There are several Creative Commons, but all CC icons found have a NC clause.
So I pick one and update the PR? This subject has already been stated before in OCA?

@sebalix
Copy link
Contributor Author

sebalix commented Apr 27, 2016

Okay, this one is a CC with commercial use allowed: http://www.icons101.com/icon/id_67712/setid_2096/Boxed_Metal_by_Martin/xlsx
PR updated with this new icon.

@pedrobaeza
Copy link
Member

👍

@pedrobaeza
Copy link
Member

Thanks!

@coveralls
Copy link

Coverage Status

Coverage increased (+0.8%) to 30.318% when pulling a51134b on osiell:8.0-report_xlsx into c368bad on OCA:8.0.

@coveralls
Copy link

coveralls commented Apr 27, 2016

Coverage Status

Coverage increased (+0.8%) to 30.318% when pulling 3c2802f on osiell:8.0-report_xlsx into c368bad on OCA:8.0.


from openerp.addons.report_xlsx.report.report_xlsx import ReportXlsx

class partner_xlsx(ReportXlsx):
Copy link
Member

Choose a reason for hiding this comment

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

a complete detail here but why not class PartnerXlsx() here ? However, It must not prevent merge

@bealdav
Copy link
Member

bealdav commented May 3, 2016

👍 thanks a lot

@mourad-ehm
Copy link
Contributor

👍 Code review

@hparfr
Copy link

hparfr commented May 3, 2016

👍

@sebastienbeau sebastienbeau merged commit bf90b56 into OCA:8.0 May 3, 2016
@sebalix sebalix deleted the 8.0-report_xlsx branch January 2, 2017 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants