[13.0][FIX] dms: Set res_mimetype correctly in files #131

victoralmau · 2021-08-20T11:04:36Z

Set res_mimetype correctly in files.

Please @yajo and @pedrobaeza can you review it?

OCA-git-bot · 2021-08-20T11:04:45Z

Hi @victoralmau! Thank you very much for this contribution. As the addon you are improving does not have a declared maintainer, I take the opportunity to mention that you can consider adopting it. To do so, please read the maintainer role description, and, if interested, create a pull request to add your GitHub login to the maintainers key of the addon manifest.

pedrobaeza

I'm not sure we really need this, as this is very partial, but deploy properly a library that detects all the possible mimetypes instead of the limited implementation inside Odoo. Please check this code:

https://github.com/odoo/odoo/blob/c5a22a122eccd18247280a941efd2c96935c43d6/odoo/tools/mimetypes.py#L165-L179

Try in your local to put such library to see if it's detected properly, and if so we should:

Add in base Doodba such library
Put in the README in the installation section the recommendation to install the library.

yajo

As @pedrobaeza said, the first thing to try is to install those libraries and see if the problem goes away automatically. If that happens, close this.

Otherwise, a couple of code comments below.

yajo · 2021-08-23T07:04:15Z

dms/models/dms_file.py

    def _compute_mimetype(self):
+        """ Similar to addons/base/models/ir_attachment.py#L232 """


This comment says nothing. And the line number can change. Please add a proper docstring and, if you need to refer to another resource, add a permalink.

yajo · 2021-08-23T07:05:41Z

dms/models/dms_file.py

+            record.res_mimetype = mimetypes.guess_type(record.name or "")[0]
+            if record.res_mimetype == "application/octet-stream":
+                record.res_mimetype = guess_mimetype(
+                    base64.b64decode(record.content or "")
+                )


IIUC, you're trying to get the mimetype from the file name and, later, from the file contents.

IMHO it should be the opposite. The contents are usually more reliable than the file extension to know the kind of file you have.

victoralmau · 2021-08-23T11:28:06Z

I tested locally to add the python-magic library and this changes will not be necessary because the res_mimetype field will be defined correctly.

Now the question is, do we add it as a dependency in the manifest or do we suggest its installation in the readme file and skip the parts of the test that are necessary if the library is not added?

Let's not forget that this happened because the res_mimetype field for .mp4 files was not defined correctly.

pedrobaeza · 2021-08-23T11:56:10Z

See my previous comment in #131 (review)

victoralmau · 2021-08-23T14:32:31Z

Changes done.
If everything is correct, do we add the python-magic library to https://github.com/Tecnativa/doodba or is it optional?

pedrobaeza · 2021-08-23T16:24:51Z

If everything is correct, do we add the python-magic library to https://github.com/Tecnativa/doodba or is it optional?

IMO we should add it for all (as it affects also to non DMS users that attach a video in chatter), but meanwhile we can add it to current instances through the regular way. cc @yajo

dms/readme/INSTALL.rst

yajo · 2021-08-24T06:43:41Z

dms/tests/test_file.py

+    def test_content_file_res_mimetype_magic_library(self):
+        if not magic:
+            self.skipTest("Without python-magic library installed")


FTR there's a decorator for this: https://docs.python.org/3/library/unittest.html#unittest.skipIf

However it's OK as it is 😊

yajo · 2021-08-24T06:44:25Z

/ocabot merge patch

OCA-git-bot · 2021-08-24T06:44:30Z

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 13.0-ocabot-merge-pr-131-by-Yajo-bump-patch, awaiting test results.

Signed-off-by Yajo

OCA-git-bot · 2021-08-24T06:57:03Z

@yajo your merge command was aborted due to failed check(s), which you can inspect on this commit of 13.0-ocabot-merge-pr-131-by-Yajo-bump-patch.

After fixing the problem, you can re-issue a merge command. Please refrain from merging manually as it will most probably make the target branch red.

Improves Odoo mime type detection out of the box. See https://github.com/odoo/odoo/blob/2dfa8fd0022b0a1fc97b721b3b8c3dd4f28889b3/odoo/tools/mimetypes.py#L164-L173 and the full story from OCA/dms#131.

yajo · 2021-08-24T08:26:45Z

Failed due to odoo/odoo#75387.

rvalyi · 2021-08-24T12:59:27Z

Hello @yajo, you could use OCA/l10n-brazil@d43e0eb
(see https://odoo-community.org/groups/contributors-15/contributors-206982?mode=date&date_begin=2021-08-01&date_end=2021-09-01 )
to workarround the issue meanwhile if you want. That being said it seems Odoo SA is moving forward with this issue so may be waiting they fix it is an option.

OCA-git-bot · 2021-08-25T11:08:54Z

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

pedrobaeza · 2021-09-03T08:16:13Z

/ocabot merge patch

It can be fw-port it to v14 as well

OCA-git-bot · 2021-09-03T08:16:23Z

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 13.0-ocabot-merge-pr-131-by-pedrobaeza-bump-patch, awaiting test results.

OCA-git-bot · 2021-09-03T08:29:19Z

Congratulations, your PR was merged at 8ae4aff. Thanks a lot for contributing to OCA. ❤️

Improves Odoo mime type detection out of the box. See https://github.com/odoo/odoo/blob/2dfa8fd0022b0a1fc97b721b3b8c3dd4f28889b3/odoo/tools/mimetypes.py#L164-L173 and the full story from OCA/dms#131. (cherry picked from commit 2332a53)

pedrobaeza added this to the 13.0 milestone Aug 20, 2021

pedrobaeza requested changes Aug 20, 2021

View reviewed changes

yajo requested changes Aug 23, 2021

View reviewed changes

victoralmau force-pushed the 13.0-fix-dms-res_mimetype_video branch 3 times, most recently from f70cab2 to 5ab267b Compare August 23, 2021 14:10

pedrobaeza reviewed Aug 23, 2021

View reviewed changes

dms/readme/INSTALL.rst Outdated Show resolved Hide resolved

[FIX] dms: Add python-magic library to readme

c563bcd

victoralmau force-pushed the 13.0-fix-dms-res_mimetype_video branch from 5ab267b to c563bcd Compare August 24, 2021 06:05

pedrobaeza approved these changes Aug 24, 2021

View reviewed changes

yajo approved these changes Aug 24, 2021

View reviewed changes

OCA-git-bot added the approved label Aug 24, 2021

OCA-git-bot added a commit that referenced this pull request Aug 24, 2021

Merge PR #131 into 13.0

a5b9159

Signed-off-by Yajo

OCA-git-bot added the bot is merging ⏳ label Aug 24, 2021

OCA-git-bot removed the bot is merging ⏳ label Aug 24, 2021

yajo mentioned this pull request Aug 24, 2021

Add python-magic Tecnativa/doodba#439

Merged

OCA-git-bot added the ready to merge label Aug 25, 2021

OCA-git-bot added the bot is merging ⏳ label Sep 3, 2021

OCA-git-bot merged commit 6d322f2 into OCA:13.0 Sep 3, 2021

OCA-git-bot added merged 🎉 and removed bot is merging ⏳ labels Sep 3, 2021

victoralmau mentioned this pull request Sep 3, 2021

[14.0][FIX] dms: Set res_mimetype correctly in files #133

Merged

yajo deleted the 13.0-fix-dms-res_mimetype_video branch September 3, 2021 11:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[13.0][FIX] dms: Set res_mimetype correctly in files #131

[13.0][FIX] dms: Set res_mimetype correctly in files #131

victoralmau commented Aug 20, 2021 •

edited

Loading

OCA-git-bot commented Aug 20, 2021

pedrobaeza left a comment

yajo left a comment

yajo Aug 23, 2021

yajo Aug 23, 2021

victoralmau commented Aug 23, 2021

pedrobaeza commented Aug 23, 2021

victoralmau commented Aug 23, 2021

pedrobaeza commented Aug 23, 2021 •

edited

Loading

yajo Aug 24, 2021

yajo commented Aug 24, 2021

OCA-git-bot commented Aug 24, 2021

OCA-git-bot commented Aug 24, 2021

yajo commented Aug 24, 2021

rvalyi commented Aug 24, 2021

OCA-git-bot commented Aug 25, 2021

pedrobaeza commented Sep 3, 2021

OCA-git-bot commented Sep 3, 2021

OCA-git-bot commented Sep 3, 2021

		def _compute_mimetype(self):
		""" Similar to addons/base/models/ir_attachment.py#L232 """

[13.0][FIX] dms: Set res_mimetype correctly in files #131

[13.0][FIX] dms: Set res_mimetype correctly in files #131

Conversation

victoralmau commented Aug 20, 2021 • edited Loading

OCA-git-bot commented Aug 20, 2021

pedrobaeza left a comment

Choose a reason for hiding this comment

yajo left a comment

Choose a reason for hiding this comment

yajo Aug 23, 2021

Choose a reason for hiding this comment

yajo Aug 23, 2021

Choose a reason for hiding this comment

victoralmau commented Aug 23, 2021

pedrobaeza commented Aug 23, 2021

victoralmau commented Aug 23, 2021

pedrobaeza commented Aug 23, 2021 • edited Loading

yajo Aug 24, 2021

Choose a reason for hiding this comment

yajo commented Aug 24, 2021

OCA-git-bot commented Aug 24, 2021

OCA-git-bot commented Aug 24, 2021

yajo commented Aug 24, 2021

rvalyi commented Aug 24, 2021

OCA-git-bot commented Aug 25, 2021

pedrobaeza commented Sep 3, 2021

OCA-git-bot commented Sep 3, 2021

OCA-git-bot commented Sep 3, 2021

victoralmau commented Aug 20, 2021 •

edited

Loading

pedrobaeza commented Aug 23, 2021 •

edited

Loading