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

Drawio improvements #6125

Merged
merged 2 commits into from
Jan 23, 2022
Merged

Drawio improvements #6125

merged 2 commits into from
Jan 23, 2022

Conversation

diocas
Copy link
Contributor

@diocas diocas commented Dec 10, 2021

A couple of improvements to Drawio:

  • Honor the autosave configuration, and actually save
  • Show error messages to the user, currently all failures are silent
  • Enable opening from public links without being authenticated

Let me know if this strategy for public links is ok. I was thinking of applying the same logic into the app provider/external app, as this is also not working properly with public links.
Maybe the mixin fileAcess that I created here can be moved somewhere else and re-used? Where do you suggest? web-pkg?

@diocas
Copy link
Contributor Author

diocas commented Dec 10, 2021

Should I fix the issue reported by sonar? I can add this.config.url to the post message (although, if there are services that redirect to a diff url this is no longer valid...)

@dschmidt
Copy link
Member

I like that you extracted the private/public link handling and I agree it should be moved out of this app.

@pascalwengerter @kulmann I've added this to the Web Fastlane as I need this kind of file handling for fixing other bugs with other apps. We should discuss what a proper place would be for this functionality and loader mixin of media-viewer - be it a mixin, a service or whatever.

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability D 1 Vulnerability
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

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

@dschmidt
Copy link
Member

Hey @diocas

#6156 has been merged. Would you mind rebasing your PR?

You could refer to the media-viewer app how to use the composable or look at my commits in #6134

Thanks! Let me know if I can support you in any way 😇

@dschmidt
Copy link
Member

Oopsie, I forgot I had applied it to draw.io already - so basically you just need to remove your file handling changes and need to make sure all other changes apply cleanly. Hope it's straight forward :)

@kulmann
Copy link
Contributor

kulmann commented Jan 21, 2022

Thanks @dschmidt - one addition: draw-io now has the :contextRouteName in the route as well, which allows us to determine if the user came from a public link or from an authenticated context. With this, the route duplication is not needed anymore.

Show proper error messages to the user
Fix loading spinner
@diocas
Copy link
Contributor Author

diocas commented Jan 21, 2022

Et voilà @dschmidt @kulmann !
Do you mind doing one last test? I'm trying but my ocis setup is broken and I still trying to understand what is going on...

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability D 1 Vulnerability
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

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

@ownclouders
Copy link
Contributor

Results for oC10SharingExternal https://drone.owncloud.com/owncloud/web/22078/40/1
The following scenarios passed on retry:

  • webUISharingExternal/federationSharing.feature:106

@diocas
Copy link
Contributor Author

diocas commented Jan 21, 2022

Looks like that with the latest changes, drawio no longer takes the configurations and just uses the default values.
edit: #6296

errorFunc(this.$gettext("Saving error. You're not authorized to save this file"))
break
default:
errorFunc(error.message || error)
Copy link
Member

Choose a reason for hiding this comment

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

We should maybe not leak any internal error message to the user, but we can fix that in a follow up PR imho.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with both statements 😅

errorFunc(this.$gettext("Saving error. You're not authorized to save this file"))
break
default:
errorFunc(error.message || error)
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with both statements 😅

@kulmann kulmann merged commit ea1b78e into owncloud:master Jan 23, 2022
@diocas diocas deleted the drawio branch January 24, 2022 08:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants