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

Create onlyoffice extension #857

Merged
merged 21 commits into from
Dec 1, 2020
Merged

Create onlyoffice extension #857

merged 21 commits into from
Dec 1, 2020

Conversation

LukasHirt
Copy link
Contributor

@LukasHirt LukasHirt commented Nov 15, 2020

Requires owncloud/web#4324 to work in ocis-web. Testing requires running oc10 server with the onlyoffice app installed and enabled + running onlyoffice docs server.

Some summary of my brain trips when creating this:

Originally I was considering creating a component which would contain an iframe loading the editor so we are not really leaving ocis-web. This had two problems though - we would need to set a new header to allow this and we would anyway see the topbar of oc10. That's why I decided to go the way of opening the editor in a new tab.

Regarding tests - I don't really see any value in tests here. We do not have any views, any big logic... If the handler is working should be tested in ocis-web on a more general level. Testing if window.open seems like a needless increase of CI time.

@update-docs

This comment has been minimized.

@LukasHirt LukasHirt force-pushed the onlyoffice-ext branch 2 times, most recently from 43dd342 to 75e41b4 Compare November 18, 2020 10:40
@LukasHirt LukasHirt marked this pull request as ready for review November 18, 2020 10:43
@LukasHirt LukasHirt self-assigned this Nov 18, 2020
@LukasHirt
Copy link
Contributor Author

Just realising now. I've actually added the extension as default into the config.json in ocis-phoenix which doesn't make sense because it won't work yet in pure ocis setup. Need to think about how do we want to deliver this in bridge setup.

Copy link
Member

@kulmann kulmann left a comment

Choose a reason for hiding this comment

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

I discovered some files that are not needed, some files that need to be one folder level higher, some occurrences of ocis-onlyoffice and some code that can be deleted.

Also, please contact @C0rby on how to implement the version command here and how to add it to the boilr template, so that we have it in every new service.

Good to see this extension! Really nice progress.

changelog/unreleased/create-onlyoffice-extension.md Outdated Show resolved Hide resolved
changelog/unreleased/create-onlyoffice-extension.md Outdated Show resolved Hide resolved
changelog/unreleased/create-onlyoffice-extension.md Outdated Show resolved Hide resolved
onlyoffice/.drone.star Outdated Show resolved Hide resolved
onlyoffice/.github/config.yml Outdated Show resolved Hide resolved
onlyoffice/ui/dev/public/index.html Outdated Show resolved Hide resolved
onlyoffice/pkg/service/v0/instrument.go Outdated Show resolved Hide resolved
onlyoffice/pkg/service/v0/logging.go Outdated Show resolved Hide resolved
onlyoffice/pkg/service/v0/service.go Outdated Show resolved Hide resolved
onlyoffice/pkg/service/v0/service.go Outdated Show resolved Hide resolved
@LukasHirt
Copy link
Contributor Author

@kulmann I've addressed all your comments. Regarding the versions command, I would like to tackle that in separate PR to finally get this one merged.

Copy link
Member

@kulmann kulmann left a comment

Choose a reason for hiding this comment

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

Fix the staticcheck, then it's good to go 🚀

@sonarcloud
Copy link

sonarcloud bot commented Dec 1, 2020

Kudos, SonarCloud Quality Gate passed!

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

No Coverage information No Coverage information
No Duplication information No Duplication information

@LukasHirt LukasHirt merged commit 0fa1d57 into master Dec 1, 2020
@LukasHirt LukasHirt deleted the onlyoffice-ext branch December 1, 2020 10:50
ownclouders pushed a commit that referenced this pull request Dec 1, 2020
Merge: 45de881 dbecc7d
Author: Lukas Hirt <lhirt@owncloud.com>
Date:   Tue Dec 1 11:50:33 2020 +0100

    Merge pull request #857 from owncloud/onlyoffice-ext
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.

2 participants