Skip to content

[15.0][MIG] attachment_preview: Migration to version 15.0 - #363

Merged
OCA-git-bot merged 47 commits into
OCA:15.0from
elego:15.0-mig-attachment_preview
Apr 18, 2023
Merged

[15.0][MIG] attachment_preview: Migration to version 15.0#363
OCA-git-bot merged 47 commits into
OCA:15.0from
elego:15.0-mig-attachment_preview

Conversation

@vancouver29

Copy link
Copy Markdown
Contributor

[15.0][MIG]attachment_preview

hbrunn and others added 30 commits August 23, 2022 11:33
In order to get visibility on https://www.odoo.com/apps the OCA board has
decided to add the OCA as author of all the addons maintained as part of the
association.
[FIX] Lint

[FIX] lint and flake

[ADD] tests

[ADD] tests

[ADD] tests

[ADD] Tests

[ADD] Package python-magic

[ADD] Tests

[FIX] Lint
Updated by "Update PO files to match POT (msgmerge)" hook in Weblate.

Translation: knowledge-11.0/knowledge-11.0-attachment_preview
Translate-URL: https://translation.odoo-community.org/projects/knowledge-11-0/knowledge-11-0-attachment_preview/
Updated by "Update PO files to match POT (msgmerge)" hook in Weblate.

Translation: knowledge-11.0/knowledge-11.0-attachment_preview
Translate-URL: https://translation.odoo-community.org/projects/knowledge-11-0/knowledge-11-0-attachment_preview/
[REM] Old files

[ADD] Magic to travis file

[FIX] Nagivation refresh widget
@vancouver29
vancouver29 force-pushed the 15.0-mig-attachment_preview branch from 17f2537 to 0bce1f3 Compare April 13, 2023 11:27
@vancouver29

Copy link
Copy Markdown
Contributor Author

I needed to change

ids_to_browse = ids if isinstance(ids, collections.Iterable) else [ids]

to

ids_to_browse = ids if isinstance(ids, collections.abc.Iterable) else [ids]

in ir_attachment.py line 20 and 69

It seams that collections.Iterable is deprecated and should be replaced by collections.abc.Iterable

@HolgerNahrstaedt I just adjusted your suggestion collections.abc.Iterableinstead of collections.Iterable
@max3903 Can we merge this PR?

@max3903

max3903 commented Apr 13, 2023

Copy link
Copy Markdown
Member

/ocabot migration attachment_preview

@OCA-git-bot OCA-git-bot mentioned this pull request Apr 13, 2023
11 tasks
@max3903 max3903 changed the title 15.0 mig attachment preview [15.0][MIG] attachment_preview: Migratn to version 15.0 Apr 13, 2023
@max3903 max3903 self-assigned this Apr 13, 2023
@max3903 max3903 changed the title [15.0][MIG] attachment_preview: Migratn to version 15.0 [15.0][MIG] attachment_preview: Migration to version 15.0 Apr 13, 2023
@max3903

max3903 commented Apr 13, 2023

Copy link
Copy Markdown
Member

@HolgerNahrstaedt Can you update your review?

@OCA-git-bot

Copy link
Copy Markdown
Contributor

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). 🤖

@HolgerNahrstaedt HolgerNahrstaedt left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Please see my suggestions with code examples. The change regarding the python code is ok.

$target = $(event.currentTarget),
split_screen = $target.attr("data-target") !== "new",
attachment_id = this.attachment.id,
attachment_extension = "pdf",

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This should be removed and a get_attachment_extension rpc call should be used instead.

@vancouver29 vancouver29 Apr 17, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@HolgerNahrstaedt we still need the variables split_screen, attachment_id as the input for the rpc request. But you are right with the attachment_extension. It is unnecessary here, so I removed it per your suggestion.

attachment_url = this.attachment.defaultSource;
active_attachment_id = attachment_id;

if (attachment_extension) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The if else path Lines 250 to 272 can be replaced by:

                rpc.query({
                    model: "ir.attachment",
                    method: "get_attachment_extension",
                    args: [attachment_id],
                }).then(function (extension) {
                    self._showPreview(
                        attachment_id,
                        attachment_url,
                        attachment_extension,
                        extension,
                        attachment_title,
                        split_screen
                    );
                });

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

it's done!

attachment_url,
attachment_extension,
attachment_title
);

@HolgerNahrstaedt HolgerNahrstaedt Apr 17, 2023

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

A check if the extensions can be preview should be added

                if (!canPreview(attachment_extension))
                {
                    return;
                }

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

it's added!

>
<div
t-if="attachmentCard.attachment.downloadUrl"
class="o_AttachmentCard_asideItem d-flex justify-content-center align-items-center ml4 o_attachment_preview"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

o_attachment_preview should be removed here

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

it's removed!

</div>
<div
t-if="attachmentCard.attachment.downloadUrl"
class="o_AttachmentCard_asideItem d-flex justify-content-center align-items-center ml4 o_attachment_preview o_attachment_preview_new_tab"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

o_attachment_preview should be removed here

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

it's removed!

holgern
holgern approved these changes Apr 18, 2023

@HolgerNahrstaedt HolgerNahrstaedt left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Sorry for the confusion, wrong account.

The changes look now good to me. Great work!

@max3903

max3903 commented Apr 18, 2023

Copy link
Copy Markdown
Member

/ocabot merge nobump

@OCA-git-bot

Copy link
Copy Markdown
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 15.0-ocabot-merge-pr-363-by-max3903-bump-nobump, awaiting test results.

@OCA-git-bot
OCA-git-bot merged commit 72440b8 into OCA:15.0 Apr 18, 2023
@OCA-git-bot

Copy link
Copy Markdown
Contributor

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

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.