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

[full-ci] Drive aliases #7430

Merged
merged 157 commits into from
Sep 29, 2022
Merged

[full-ci] Drive aliases #7430

merged 157 commits into from
Sep 29, 2022

Conversation

dschmidt
Copy link
Member

@dschmidt dschmidt commented Aug 10, 2022

Description

We changed the URL format to not use storageIds in the URL path anymore to identify spaces, but instead use drive aliases of spaces in the URL path.

BREAKING CHANGE for users: this breaks existing bookmarks - they won't resolve anymore.
BREAKING CHANGE for developers: the appDefaults composables from web-pkg now work with drive aliases, concatenated with relative item paths, instead of webdav paths. If you use the appDefaults composables in your application it's likely that your code needs to be adapted.

Related Issue

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:

  • use drive aliases in trash routes
  • use drive aliases in app routes
  • interpret public link token as drive alias
  • replace usages of the storageId route param with the id of the resolved drive item
  • get rid of all occurrences of route names files-spaces-personal and files-spaces-project
  • check and fix right sidebar in all views
  • check and fix file actions in all views
  • grep for routes with driveAliasAndItem and add shareId to route query
  • discuss the public link files route (duplicates public path segment) -> files/link/${driveAliasAndItem} & files/upload/${token}
  • Personal space initial load -> New and Upload buttons are disabled
  • double check upload composables (some dead code?)
  • grep for storageId or currentStorageId or item in context of routes (should not exist anymore)
  • UploadInfo.vue -> createFolderLink fix wrong route params
  • webDavPath handling: spaces should have a function to join webDavPath/Url with a path (could also strip public-files for sdk usage)

Followups (with issues)

Followups (issues to be created)

  • web-app-draw-io: modifies filePath for visio files but not the fileContext: wrong route, saving and permission checks possibly don't work...
  • review/improve types for web-client.webdav/appDefaults composables
  • redirect public links to upload/link URL if their role was changed later (resolve public link, change role, reload page)
  • port loadPreview to space + path
  • cleanup mitigated storageId absence in oc10

@update-docs
Copy link

update-docs bot commented Aug 10, 2022

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.

@kulmann kulmann force-pushed the drive-aliases branch 3 times, most recently from 8a19831 to 056a3c3 Compare September 8, 2022 07:34
@kulmann kulmann force-pushed the drive-aliases branch 2 times, most recently from 9fc5112 to 28e13a7 Compare September 22, 2022 14:17
): Promise<Resource[]> {
let resources: Resource[]
if (isPublicSpaceResource(space)) {
resources = await sdk.publicFiles.list(
`${space.webDavPath.replace(/^\/public-files/, '')}/${path || ''}`,
space.publicLinkPassword,
DavProperties.PublicLink,
[DavProperties.PublicLink].concat(davProperties),
Copy link
Member Author

@dschmidt dschmidt Sep 23, 2022

Choose a reason for hiding this comment

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

Does this make sense? Do we always want to add this? Is it harmful to pass it to private files?
If we don't add it by default, I'm wondering how API consumers are supposed to add it if they need it?

Another approach would be to filter it out for non public link files.

Copy link
Member

Choose a reason for hiding this comment

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

no, if you provide dav properties they should completely replace the default dav properties. Otherwise it would not be possible to do a propfind with just the id prop for example. All other requested data would be useless and bloat the response payload.

Copy link
Member Author

Choose a reason for hiding this comment

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

Should we remove the PublicLink property from davProperties for non PublicLink spaces?
Or doesn't it harm to request it even if it does not make sense?

I'm think of requesting additional properties for a resource and not having to think about whether we're dealing with a public link... does that make sense at all? I haven't worked with the webdav api really, so it's hard for me to judge...

Copy link
Member Author

Choose a reason for hiding this comment

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

I've replaced the code with davProperties || [DavProperties.PublicLink]. Still wondering, should we filter the PublicLink property out for the private files?

@ownclouders
Copy link
Contributor

ownclouders commented Sep 23, 2022

Results for oc10SharingIntUsers3 https://drone.owncloud.com/owncloud/web/28741/29/1

💥 The acceptance tests failed on retry. Please find the screenshots inside ...

webUISharingInternalUsersShareWithPage-shareWithUsers_feature-L89.png

webUISharingInternalUsersShareWithPage-shareWithUsers_feature-L89.png

@JammingBen
Copy link
Collaborator

JammingBen commented Sep 26, 2022

My findings so far:

  • Indirect share indicators are missing in files table
  • Console errors on „Shared with me page“: Missing required prop: "space"
  • „Shared via link“ view is is broken (endless reload)
  • Sidebar is broken on „Shared with me/others“ page
  • Resources within a space don’t show space members in sidebar
  • „New/Upload“-buttons are missing on a public link with „edit“-permissions on root

@JammingBen
Copy link
Collaborator

JammingBen commented Sep 26, 2022

Some more:

  • Open sidebar in/for the root of personal space: sidebar for a project space is showing (space image, members) -> This is related to the deactivated „New/Upload“-buttons-issue (a.k.a. missing slash).
  • Sidebar for public link root shows space sidebar (same as on personal view)
  • Sidebar for resources on a public link shows „Select a file or folder to view details.“
  • A strange one that only happens from time to time: right click file -> Delete -> right click another file -> context menu breaks: TypeError: Cannot read properties of undefined (reading '$el') -> doesn't happen anymore, I assume it has been fixed.
  • Navigate from the trashbin to personal space -> PROPFIND 404 -> this happens because of the sidebar resource fetching. We do this as a follow up. Also see [full-ci] Space root not updated #7546 which should fix this
  • Navigate into a subfolder, delete the subfolder via the context menu in the breadcrumbs -> redirect to parent folder is broken
  • Sidebar & context menu on search result page is broken
  • Breadcrumb in the trashbin shows context menu -> should not show
  • „Delete“-action in share jail results in AxiosError in console (delete succeeds however)
  • "Shared with me"-page: Resource name in sidebar is missing, ID is showing instead:
    image
  • Reshare a received share -> open the share in share jail -> open the share-sidebar-panel for the current folder (via breadcrumb menu) -> the reshare is missing
  • Renaming resources on "Shared with me/via link" pages via the pencil (next to the name) is broken
  • Right sidebar in "Shared via link"-page shows console error TypeError: Cannot read properties of undefined (reading 'split')
  • Create a public link for a received share -> go to "Shared via link"-page -> contextmenu + sidebar is broken for this resource
  • Parent folder in upload modal is missing when uploading to personal root

@AlexAndBear
Copy link
Contributor

AlexAndBear commented Sep 26, 2022

  • Batch download doesn't work in public link view, click on download button results in [Vue warn]: Error in v-on handler (Promise/async): "TypeError: Cannot read properties of undefined (reading 'split')"
  • Trashbin -> Click side bar toggle, select a file, deselect the file -> restore and delete action on trash space possible but shouldn't -> broken on master as well. I've created Disable sidebar in trashbin #7695
  • Delete a file in personal home -> Navigate to trashbin -> Click on parentFolder 'Personal' on the deleted resource -> Breadcrumb contains a '.' in folder structure

@JammingBen
Copy link
Collaborator

JammingBen commented Sep 27, 2022

Findings with oc10:

  • "Create/Upload"-buttons on public link page are missing
  • Links to resources (and parent folders) in "Shared with others/via links" views are broken ("Open"-action as well)
  • Shares for resources in "Shared with others/via links" views are missing in sidebar
  • Favorites View is broken
  • Upload to oC10 doesn't work
  • Rename in "Shared with me" page doesn't work
  • Search links don't work (search result modal & search file list)

@AlexAndBear
Copy link
Contributor

AlexAndBear commented Sep 27, 2022

OCIS followup findings:

  • Copy paste in space root, doesn't work -> conflict dialog not shown + precondition failed
  • Shared via link -> shared folder in root doesn't open click, neither parent folder is clickable (wrong uri)

JammingBen and others added 4 commits September 28, 2022 18:26
The item path was not respected by the new DriveRedirect component.
Changed that, mostly so that acceptance tests can directly route to a
specific folder without providing the drive alias.
Additionally, the DriveRedirect component has been stripped from
irrelevant use cases.
@kulmann kulmann marked this pull request as ready for review September 29, 2022 09:37
@sonarcloud
Copy link

sonarcloud bot commented Sep 29, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability D 5 Vulnerabilities
Security Hotspot E 4 Security Hotspots
Code Smell A 31 Code Smells

32.0% 32.0% Coverage
1.7% 1.7% Duplication

@kulmann kulmann merged commit 401a981 into master Sep 29, 2022
@delete-merged-branch delete-merged-branch bot deleted the drive-aliases branch September 29, 2022 10:48
@pascalwengerter
Copy link
Contributor

Congrats on merging this beast 🥳

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.

Root folder in share jail is "undefined" in breadcrumbs Enable navigation by driveAlias in the Web UI
7 participants