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.

Sign up for GitHub

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

Jump to bottom

[full-ci] Internalize app provider #5805

Merged
merged 4 commits into from
Oct 1, 2021
Merged

[full-ci] Internalize app provider #5805

merged 4 commits into from
Oct 1, 2021

Conversation

pascalwengerter
Copy link
Contributor

@pascalwengerter pascalwengerter commented Sep 14, 2021 •
edited
Loading

Description

Supersedes #5712

Related Issue

Types of changes

  • New feature (non-breaking change which adds functionality)

Checklist:

@update-docs
Copy link

update-docs bot commented Sep 14, 2021

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

@pascalwengerter pascalwengerter force-pushed the 09092021_app-provider branch 4 times, most recently from d835fa5 to 091904e Compare September 20, 2021 12:17
@ownclouders
Copy link
Contributor

Results for oC10SharingInternalGroupsSharingIndicator https://drone.owncloud.com/owncloud/web/19115/24/1
The following scenarios passed on retry:

  • webUISharingInternalGroupsToRootSharingIndicator/shareWithGroups.feature:104
    💥 The acceptance tests pipeline failed. The build has been cancelled.

@pascalwengerter pascalwengerter mentioned this pull request Sep 21, 2021
Closed
7 tasks
kulmann
kulmann requested changes Sep 21, 2021
View reviewed changes
packages/web-app-external/src/App.vue Outdated Show resolved Hide resolved
packages/web-app-external/src/App.vue Show resolved Hide resolved
packages/web-app-external/src/App.vue Outdated Show resolved Hide resolved
packages/web-app-external/src/components/LoadingScreen.vue Outdated Show resolved Hide resolved
packages/web-app-external/src/store/index.ts Outdated Show resolved Hide resolved
packages/web-app-external/src/store/index.ts Outdated Show resolved Hide resolved
packages/web-app-external/tests/unit/app.spec.js Outdated Show resolved Hide resolved
@pascalwengerter pascalwengerter force-pushed the 09092021_app-provider branch 2 times, most recently from 68239a2 to 091d53d Compare September 29, 2021 13:44
@pascalwengerter pascalwengerter marked this pull request as ready for review September 29, 2021 13:46
@pascalwengerter
Copy link
Contributor Author

To test this, please use the dockerized setup from https://github.com/owncloud/ocis/tree/master/deployments/examples/ocis_wopi, add the "external" app to the web config and link the web from this PR

@ownclouders
Copy link
Contributor

Results for oCISSharingInternalGroups https://drone.owncloud.com/owncloud/web/19280/54/1
💥 The acceptance tests pipeline failed. The build has been cancelled.

@ownclouders
Copy link
Contributor

Results for oCISBasic https://drone.owncloud.com/owncloud/web/19282/47/1
💥 The acceptance tests pipeline failed. The build has been cancelled.

@ownclouders
Copy link
Contributor

Results for oCISBasic https://drone.owncloud.com/owncloud/web/19283/47/1
💥 The acceptance tests pipeline failed. The build has been cancelled.

@pascalwengerter pascalwengerter changed the title Internalize app provider [full-ci] Internalize app provider Sep 29, 2021
@pascalwengerter pascalwengerter mentioned this pull request Sep 29, 2021
Closed
@ownclouders
Copy link
Contributor

Results for oC10SharingInternalUsersSharingIndicator https://drone.owncloud.com/owncloud/web/19290/28/1
The following scenarios passed on retry:

  • webUISharingInternalUsersSharingIndicator/shareWithUsers.feature:154

@ownclouders
Copy link
Contributor

Results for oC10SharingAccept https://drone.owncloud.com/owncloud/web/19290/11/1
The following scenarios passed on retry:

  • webUISharingAcceptSharesToRoot/acceptShares.feature:108

wkloucek
wkloucek reviewed Sep 29, 2021
View reviewed changes
packages/web-app-files/src/mixins/fileActions.js
const availableExternalAppActions = this.$_fileActions_loadApps(resource)

for (const action of availableExternalAppActions) {
action.handler = () => this.$_fileActions_openLink(action.name, resource.fileId)
Copy link
Contributor

Choose a reason for hiding this comment

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

If we move the app name from the route (`ocis.owncloud.test/#/external/Collabora/MTI4....) in a query parameter (ocis.owncloud.test/#/external/MTI4...?app=Collabora), we could skip it.

Calling /app/open without a app_name query parameter already opens it in the default application.
Additionally we also could add information on the /app/list endpoint, which application is the default.

@wkloucek
Copy link
Contributor

Great work, works like a charm for me! I like it especially with the right-click-menu 😍

Having a meaning full title bar would be great though. The current text is not really helpfull:
image

@wkloucek wkloucek mentioned this pull request Sep 30, 2021
Closed
4 tasks
wkloucek
wkloucek reviewed Sep 30, 2021
View reviewed changes
packages/web-app-external/tests/unit/app.spec.js
}
}

const providerSuccessResponseGet = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Responses with a method GET may also provide headers.

{
    "app-url": "jupyterlabs.owncloud.com/src=...",
    "method": "GET",
    "headers": {
        "user-language": "en",
        "user-session-id": 1234
    }
}

Source: cs3org/reva#1923 (comment), https://github.com/cs3org/go-cs3apis/blob/cb9e3c99f8dea052b9395ea84d468c0c3cfa93e7/cs3/app/provider/v1beta1/resources.pb.go#L38

@wkloucek
Copy link
Contributor

One more idea: I would find it helpful to have an ownCloud logo in the top bar, so that I clearly notice that I am within ownCloud.

For eg. CodiMD this is currently a bit hard in my opinion.

image

Collabora on oC10 looks like that, an you clearly notice that you're within ownCloud
image

@wkloucek
Copy link
Contributor

During testing I also had some hidden right click menus, but this should be an separate ticket?

Peek.2021-09-30.09-06.mp4

@pascalwengerter
Copy link
Contributor Author

During testing I also had some hidden right click menus, but this should be an separate ticket?
Peek.2021-09-30.09-06.mp4

Thanks, already tracked in #5845

@pascalwengerter
Copy link
Contributor Author

One more idea: I would find it helpful to have an ownCloud logo in the top bar, so that I clearly notice that I am within ownCloud.

For eg. CodiMD this is currently a bit hard in my opinion.

image

Collabora on oC10 looks like that, an you clearly notice that you're within ownCloud image

Agreed, I've always kinda been waiting for the great redesign to finally get around doing this. Feel free to open a dedicated ticket (in the ODS) since this is independent of the app provider ;)

@wkloucek
Copy link
Contributor

I just saw that there are always scrollbars on the right and that you don't see the full iframe. Looks like the iframe is too high.

Peek.2021-09-30.12-55.mp4

kulmann
kulmann requested changes Sep 30, 2021
View reviewed changes
config/config.json.sample-ocis Show resolved Hide resolved
dev/docker/ocis.web.config.json Outdated Show resolved Hide resolved
packages/web-app-external/package.json Outdated Show resolved Hide resolved
packages/web-app-external/src/index.js Outdated Show resolved Hide resolved
@sonarcloud
Copy link

sonarcloud bot commented Oct 1, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 5 Code Smells

58.6% 58.6% Coverage
0.0% 0.0% Duplication

@kulmann kulmann merged commit 70df15c into master Oct 1, 2021
@delete-merged-branch delete-merged-branch bot deleted the 09092021_app-provider branch October 1, 2021 08:49
@wkloucek wkloucek mentioned this pull request Oct 1, 2021
Closed
@pascalwengerter pascalwengerter mentioned this pull request Oct 1, 2021
Open
This was referenced Oct 29, 2021
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wkloucek wkloucek wkloucek left review comments

@kulmann kulmann kulmann approved these changes

@fschade fschade Awaiting requested review from fschade

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

<hr> inside <ul> in ContextMenu is an a11y-offense
4 participants
@pascalwengerter @ownclouders @wkloucek @kulmann