[14.0][MIG] document_url: Migration to 14.0 by victoralmau · Pull Request #306 · OCA/knowledge

victoralmau · 2021-10-13T07:48:33Z

Supersede from: #304

Migration to 14.0.

Extra changes:

Define the add attachment and add URL buttons on the same line.
Allow downloading of url type attachments by clicking on the icon or the name.
Define the name of the url type attachments as links (allows to open it in a new tab).
Hide the download button for url attachments.
Force to set mimetype to "application/link" for url attachments.
Displays a link icon for url attachments.

Please @Tardo and @pedrobaeza can you review it?

[MIG] document_url * bump version to 9.0.1.0.0 and make it installable * Update JS structure * Add css in order to be compliant with odoo style * Add a README.rst and OCA conventions * Improve selector * add fr.po for fr translation * open link with target='_blank'

OCA Transbot updated translations from Transifex

Without this patch, when you hover on the menu item, its background color does not change, and its cursor is a text one. With this patch, the menu item styling matches those available in all surrounding menus.

Replace char © by word Copyright Change format in usage description [UPD] Update document_url.pot [UPD] README.rst Update translation files Updated by Update PO files to match POT (msgmerge) hook in Weblate.

[UPD] README.rst [UPD] Update document_url.pot Update translation files Updated by "Update PO files to match POT (msgmerge)" hook in Weblate. Translation: knowledge-12.0/knowledge-12.0-document_url Translate-URL: https://translation.odoo-community.org/projects/knowledge-12-0/knowledge-12-0-document_url/ [UPD] README.rst

Currently translated at 100.0% (14 of 14 strings) Translation: knowledge-12.0/knowledge-12.0-document_url Translate-URL: https://translation.odoo-community.org/projects/knowledge-12-0/knowledge-12-0-document_url/hr/

[UPD] Update document_url.pot [UPD] README.rst Update translation files Updated by "Update PO files to match POT (msgmerge)" hook in Weblate. Translation: knowledge-13.0/knowledge-13.0-document_url Translate-URL: https://translation.odoo-community.org/projects/knowledge-13-0/knowledge-13-0-document_url/

document_url 13.0.1.0.1

[MIG] document_url: Migration to 14.0

TT30263

pedrobaeza

Functional review:

The "Add URL" button link appears in a new line, taking more space, instead of appearing in the same line as previous versions.
Although it's not explicitly something of this module, the way URL attachments now behave seems odd: they appear as a regular attachment, but with a generic icon (non clickable), and the download button. Only clicking on such button leads to open the URL in the same tab, but it's not possible to do ctrl+click (or third-button click) to open in new tab. I think this module should add some changes to this behavior to have a special icon like in previous versions, and be totally clickable, opening the link in a new tab.

pedrobaeza · 2021-10-13T19:04:09Z

This is better, thanks, but I'm still unable to open it in a new tab, and it's opened by default in the same tab. The download icon can be also hidden in this type of attachment. A last thing: about the icon, isn't a more suitable icon for the link?

victoralmau · 2021-10-14T08:57:11Z

Some changes have been made.

There are other pending changes that would be quite difficult to make (it would require overwriting quite a bit of core code):

Change the icon of url type attachments (in v13 it was the same).
Change the icon and define it as a link (now an on-click is defined that does not allow it to be opened in a new tab).

pedrobaeza · 2021-10-14T09:01:28Z

Then what are the changes you have made? Are you going to handle the rest? At least we need to do ctrl+click for opening in new tab.

victoralmau · 2021-10-14T09:04:54Z

Then what are the changes you have made? Are you going to handle the rest? At least we need to do ctrl+click for opening in new tab.

Sorry, I have not mentioned the changes (if they are in the commit).
The changes that are already made are the following:

Hide the download button in url type attachments.
Define the name of the attachment as a link, this allows to open it in a new tab with Ctrl + Click.

pedrobaeza · 2021-10-14T15:29:48Z

This is better, thanks, although the other features are very desired. Isn't possible to add to the link (<a> tag?) the attribute target="new" for having the new tab thing? Another thing is that the target should be the URL, not the controller path that is later resolved to the link.

victoralmau · 2021-10-14T15:42:46Z

Some more clarifications regarding the previous comment::

Yes, it is possible to add target="new" to the link.
It is not possible to add attachment.url directly because that field was not get from https://github.com/odoo/odoo/blob/14.0/addons/mail/static/src/models/thread/thread.js#L680
The icon cannot be changed (it is not really an icon, it is a div that has as its backround the mimetype https://github.com/odoo/odoo/blob/14.0/addons/mail/static/src/components/attachment /attachment.xml#L17-L25)

I think one possibility would be to suggest some changes to Odoo to help with this, what do you think?

pedrobaeza · 2021-10-14T15:44:43Z

Odoo is not going to change anything about this, as URLs are not allowed anywhere in the UI. Aren't you able to intercept the mimetype compute method for putting a special one, and provide the mimetype icon here in this module instead on the main one?

victoralmau · 2021-10-15T14:19:43Z

I have added all the extra changes to the PR description to clarify all of them.

pedrobaeza · 2021-10-15T15:41:01Z

Great, we are advancing a lot, but now you have hidden too much, as for PDFs, the download button nor the extension is shown (after refreshing one time the tab):

On this PR:

On runbot:

pedrobaeza · 2021-10-15T18:43:32Z

Now the download button has appeared again for links.

Define the add attachment and add URL buttons on the same line. Allow downloading of url type attachments by clicking on the icon or the name. Define the name of the url type attachments as links (allows to open it in a new tab). Hide the download button for url attachments. Force to set mimetype to "application/link" for url attachments. Displays a link icon for url attachments. TT30263

victoralmau · 2021-10-18T06:06:46Z

Now the download button has appeared again for links.

Sorry for the absurd confusion, it's already changed.

pedrobaeza

Now it's working as desired.

Tardo

LGTM 👍 Only code review.

The mimetype for attachments of type url seems not to be relevant. The field is calculated when you write/create and on the other hand the mimetype 'application/link' doesn't seem to be valid (https://www.iana.org/assignments/media-types/media-types.xhtml). So if you really want to force a mimetype... it might be more convenient to use application/octet-stream

pedrobaeza · 2021-10-21T15:29:42Z

Let's move on for now:

/ocabot merge nobump

I think the mimetype helps for displaying such icon.

OCA-git-bot · 2021-10-21T15:30:00Z

On my way to merge this fine PR!
Prepared branch 14.0-ocabot-merge-pr-306-by-pedrobaeza-bump-nobump, awaiting test results.

Signed-off-by pedrobaeza

CasVissers-360ERP · 2021-11-06T08:34:35Z

@pedrobaeza Merge seems stalled?

pedrobaeza · 2021-11-06T09:20:19Z

/ocabot merge nobump

OCA-git-bot · 2021-11-06T09:20:30Z

On my way to merge this fine PR!
Prepared branch 14.0-ocabot-merge-pr-306-by-pedrobaeza-bump-nobump, awaiting test results.

OCA-git-bot · 2021-11-06T09:54:49Z

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

pedrobaeza and others added 16 commits September 1, 2021 13:48

[ADD] document_url: Module to attach URLs as documents

65f3681

Slovene translations added

c78bada

[FIX] document_url: Fix documentation

9f6f42f

OCA Transbot updated translations from Transifex

[FIX][document_url] Consistent styling.

d95f436

Without this patch, when you hover on the menu item, its background color does not change, and its cursor is a text one. With this patch, the menu item styling matches those available in all surrounding menus.

[MIG] document_url: Migration to 11.0

348420f

[MIG] document_url: Refactoring code

b9c31db

Replace char © by word Copyright Change format in usage description [UPD] Update document_url.pot [UPD] README.rst Update translation files Updated by Update PO files to match POT (msgmerge) hook in Weblate.

[FIX] pylint/javascript-lint

d0a4b66

Translated using Weblate (Croatian)

2c9b491

Currently translated at 100.0% (14 of 14 strings) Translation: knowledge-12.0/knowledge-12.0-document_url Translate-URL: https://translation.odoo-community.org/projects/knowledge-12-0/knowledge-12-0-document_url/hr/

[IMP] document_url: black, isort

9e1c16c

[FIX] document_url: Fix module dependencies.

4681064

document_url 13.0.1.0.1

[IMP] document_url: black, isort, prettier

4cd9c56

[MIG] document_url: Migration to 14.0

e0a223f

[MIG] document_url: Migration to 14.0

[MIG] document_url: Migration to 14.0

ba14237

TT30263

pedrobaeza added this to the 14.0 milestone Oct 13, 2021

pedrobaeza requested changes Oct 13, 2021

View reviewed changes

victoralmau force-pushed the 14.0-mig-document_url branch from 836cfb3 to 4bf2679 Compare October 14, 2021 08:54

victoralmau force-pushed the 14.0-mig-document_url branch from 4bf2679 to 480f502 Compare October 14, 2021 15:43

victoralmau force-pushed the 14.0-mig-document_url branch from 480f502 to edfe496 Compare October 15, 2021 14:05

victoralmau force-pushed the 14.0-mig-document_url branch from edfe496 to 68143d1 Compare October 15, 2021 15:44

victoralmau force-pushed the 14.0-mig-document_url branch from 68143d1 to 0669b94 Compare October 18, 2021 06:05

pedrobaeza approved these changes Oct 18, 2021

View reviewed changes

pedrobaeza requested a review from Tardo October 18, 2021 11:55

Tardo approved these changes Oct 21, 2021

View reviewed changes

OCA-git-bot added the approved label Oct 21, 2021

OCA-git-bot added the bot is merging ⏳ label Oct 21, 2021

OCA-git-bot added a commit that referenced this pull request Oct 21, 2021

Merge PR #306 into 14.0

cb3d413

Signed-off-by pedrobaeza

OCA-git-bot merged commit 72656e5 into OCA:14.0 Nov 6, 2021

OCA-git-bot added merged 🎉 and removed bot is merging ⏳ labels Nov 6, 2021

yajo deleted the 14.0-mig-document_url branch November 8, 2021 11:26

victoralmau mentioned this pull request Jul 18, 2022

[MIG] document_url: Migration to 15.0 #353

Merged

Uh oh!

Uh oh!

Conversation

victoralmau commented Oct 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pedrobaeza left a comment

Choose a reason for hiding this comment

Uh oh!

pedrobaeza commented Oct 13, 2021

Uh oh!

victoralmau commented Oct 14, 2021

Uh oh!

pedrobaeza commented Oct 14, 2021

Uh oh!

victoralmau commented Oct 14, 2021

Uh oh!

pedrobaeza commented Oct 14, 2021

Uh oh!

victoralmau commented Oct 14, 2021

Uh oh!

pedrobaeza commented Oct 14, 2021

Uh oh!

victoralmau commented Oct 15, 2021

Uh oh!

pedrobaeza commented Oct 15, 2021

Uh oh!

pedrobaeza commented Oct 15, 2021

Uh oh!

victoralmau commented Oct 18, 2021

Uh oh!

pedrobaeza left a comment

Choose a reason for hiding this comment

Uh oh!

Tardo left a comment

Choose a reason for hiding this comment

Uh oh!

pedrobaeza commented Oct 21, 2021

Uh oh!

OCA-git-bot commented Oct 21, 2021

Uh oh!

CasVissers-360ERP commented Nov 6, 2021

Uh oh!

pedrobaeza commented Nov 6, 2021

Uh oh!

OCA-git-bot commented Nov 6, 2021

Uh oh!

OCA-git-bot commented Nov 6, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

victoralmau commented Oct 13, 2021 •

edited

Loading