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

Use default app from app provider and early returns in fileActions #5970

Merged
merged 1 commit into from
Nov 16, 2021

Conversation

kulmann
Copy link
Member

@kulmann kulmann commented Nov 2, 2021

Description

Switch to early returns when evaluating the default action for a file. Also use the app provider without app param so that the app provider can decide on its own which app to use.

Related Issue

Motivation and Context

Improve default action handling for files.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

Open tasks:

@update-docs

This comment has been minimized.

@kulmann kulmann marked this pull request as draft November 2, 2021 11:37
@kulmann kulmann requested a review from wkloucek November 2, 2021 11:37
@kulmann
Copy link
Member Author

kulmann commented Nov 2, 2021

At the moment the POST to e.g. https://ocis.owncloud.test/app/open?file_id=MTI4NGQyMzgtYWE5Mi00MmNlLWJkYzQtMGIwMDAwMDA5MTU3OjFkZTU3Yzg1LWY0NDItNDhjZS1iZWE4LTQzMjZkZmNjOGNjYw== (= without app name) fails with a http 500 with message error searching for app provider - do you know details @wkloucek before I start to dig deeper?

@wkloucek
Copy link
Contributor

wkloucek commented Nov 2, 2021

At the moment the POST to e.g. https://ocis.owncloud.test/app/open?file_id=MTI4NGQyMzgtYWE5Mi00MmNlLWJkYzQtMGIwMDAwMDA5MTU3OjFkZTU3Yzg1LWY0NDItNDhjZS1iZWE4LTQzMjZkZmNjOGNjYw== (= without app name) fails with a http 500 with message error searching for app provider - do you know details @wkloucek before I start to dig deeper?

Thanks for raising this. I had a look at the code and there is a bug regarding the default app handling.

Besides that we also have a conceptual issue regarding default apps. The backend has cases where there is no default app, but how do clients like Web know? Currently they just get an error.

@kulmann
Copy link
Member Author

kulmann commented Nov 3, 2021

Besides that we also have a conceptual issue regarding default apps. The backend has cases where there is no default app, but how do clients like Web know? Currently they just get an error.

Hmm... actually, now thinking about this, I don't want to call the open endpoint without app name anymore. We currently have three kinds of file handlers in this order:
1: apps which are directly added to the web config (e.g. mediaviewer)
2: apps from the app provider
3: system actions (e.g. download or open in new tab, which we do for PDF files)

If there is no default in (2) we should continue in the list and pick a system action as default. There is one in any case (download).

@kulmann
Copy link
Member Author

kulmann commented Nov 3, 2021

This also means that we need the info about the default app from the app provider @wkloucek

@wkloucek
Copy link
Contributor

wkloucek commented Nov 3, 2021

This also means that we need the info about the default app from the app provider @wkloucek

cs3org/reva#2230 implements that and also fixes your error

@wkloucek
Copy link
Contributor

@kulmann owncloud/ocis#2726 will bring cs3org/reva#2230 into oCIS to unblock this PR.

@pascalwengerter pascalwengerter mentioned this pull request Nov 11, 2021
26 tasks
@kulmann kulmann force-pushed the use-default-app-from-app-provider branch from 5b13d66 to 987ff76 Compare November 11, 2021 15:13
@ownclouders
Copy link
Contributor

Results for oC10SharingPublicExpireAndRoles https://drone.owncloud.com/owncloud/web/20251/37/1
💥 The acceptance tests pipeline failed. The build has been cancelled.

@kulmann
Copy link
Member Author

kulmann commented Nov 12, 2021

Not blocked anymore thanks to @wkloucek ❤️

@kulmann kulmann force-pushed the use-default-app-from-app-provider branch from 987ff76 to 52613b0 Compare November 12, 2021 10:05
@kulmann kulmann marked this pull request as ready for review November 12, 2021 10:05
Copy link
Contributor

@pascalwengerter pascalwengerter left a comment

Choose a reason for hiding this comment

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

First test with the wopi example revealed an TypeError: action.handler is not a function when clicking on an .odt file. Investigating further

@pascalwengerter
Copy link
Contributor

First test with the wopi example revealed an TypeError: action.handler is not a function when clicking on an .odt file. Investigating further

Also, opening external apps via context menu fails with Error fetching app information and statuscode 500

@kulmann kulmann force-pushed the use-default-app-from-app-provider branch from de8a1ef to 8ed3113 Compare November 15, 2021 15:49
@kulmann kulmann force-pushed the use-default-app-from-app-provider branch from 8ed3113 to a04ba51 Compare November 15, 2021 15:53
@sonarcloud
Copy link

sonarcloud bot commented Nov 15, 2021

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

11.5% 11.5% Coverage
0.0% 0.0% Duplication

@kulmann
Copy link
Member Author

kulmann commented Nov 15, 2021

First test with the wopi example revealed an TypeError: action.handler is not a function when clicking on an .odt file. Investigating further

Also, opening external apps via context menu fails with Error fetching app information and statuscode 500

Thanks, fixed it!

Copy link
Contributor

@pascalwengerter pascalwengerter left a comment

Choose a reason for hiding this comment

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

Works like a charm now 🧮

@kulmann kulmann merged commit c0e09f4 into master Nov 16, 2021
@delete-merged-branch delete-merged-branch bot deleted the use-default-app-from-app-provider branch November 16, 2021 08:12
@kulmann kulmann mentioned this pull request Dec 2, 2021
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.

use default app for external apps when not specifying one
4 participants