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] Resource ids in urls #7725

Merged
merged 68 commits into from
Oct 10, 2022
Merged

[full-ci] Resource ids in urls #7725

merged 68 commits into from
Oct 10, 2022

Conversation

kulmann
Copy link
Member

@kulmann kulmann commented Sep 30, 2022 •
edited

Description

Related Issue

Motivation and Context

How Has This Been Tested?

  • test environment:
  • test case 1:
  • test case 2:
  • ...

Screenshots (if appropriate):

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:

  • add parentFolderId in all buildResource helpers.
  • fix routes in upload related source files (UploadInfo.vue, useUpload.ts, useUploaderHelpers.ts)
  • remove code duplication in multiple folder loaders: can be merged into one folder loader.
  • use webdav meta request to get file path by file id in folder loader (if loading by path fails) and try again
  • rewrite route to correct path if invalid
  • appDefaults: use fileId in FileContext if present
  • changelog items
  • repeated click on nav items doesn't re-resolve the correct space (for personal/all files and for trash), only for oc10.

Followup tasks / out of scope for this PR

  • breadcrumbs are currently built from parent paths segments, we'd need a propfind to each parent folder to build routes with fileIds (if we'd switched to propfinds for gathering share type information, we'd already have that... now it bites us. need to find a mitigation).

@update-docs
Copy link

update-docs bot commented Sep 30, 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 resource-ids-in-urls branch 5 times, most recently from 4b2d884 to bf63c45 Compare October 5, 2022 08:10
@ownclouders
Copy link
Contributor

ownclouders commented Oct 6, 2022 •
edited

Results for oC10Files3 https://drone.owncloud.com/owncloud/web/28985/19/1

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

webUIPreview-mediaPreview_feature-L106.png

webUIPreview-mediaPreview_feature-L106.png

webUIPreview-mediaPreview_feature-L114.png

webUIPreview-mediaPreview_feature-L114.png

webUIPreview-mediaPreview_feature-L122.png

webUIPreview-mediaPreview_feature-L122.png

webUIPreview-mediaPreview_feature-L138.png

webUIPreview-mediaPreview_feature-L138.png

webUIPreview-mediaPreview_feature-L139.png

webUIPreview-mediaPreview_feature-L139.png

webUIPreview-mediaPreview_feature-L14.png

webUIPreview-mediaPreview_feature-L14.png

webUIPreview-mediaPreview_feature-L140.png

webUIPreview-mediaPreview_feature-L140.png

webUIPreview-mediaPreview_feature-L143.png

webUIPreview-mediaPreview_feature-L143.png

webUIPreview-mediaPreview_feature-L15.png

webUIPreview-mediaPreview_feature-L15.png

webUIPreview-mediaPreview_feature-L153.png

webUIPreview-mediaPreview_feature-L153.png

webUIPreview-mediaPreview_feature-L16.png

webUIPreview-mediaPreview_feature-L16.png

webUIPreview-mediaPreview_feature-L18.png

webUIPreview-mediaPreview_feature-L18.png

webUIPreview-mediaPreview_feature-L25.png

webUIPreview-mediaPreview_feature-L25.png

webUIPreview-mediaPreview_feature-L35.png

webUIPreview-mediaPreview_feature-L35.png

webUIPreview-mediaPreview_feature-L56.png

webUIPreview-mediaPreview_feature-L56.png

webUIPreview-mediaPreview_feature-L57.png

webUIPreview-mediaPreview_feature-L57.png

webUIPreview-mediaPreview_feature-L60.png

webUIPreview-mediaPreview_feature-L60.png

webUIPreview-mediaPreview_feature-L68.png

webUIPreview-mediaPreview_feature-L68.png

webUIPreview-mediaPreview_feature-L77.png

webUIPreview-mediaPreview_feature-L77.png

webUIPreview-mediaPreview_feature-L84.png

webUIPreview-mediaPreview_feature-L84.png

webUIPreview-mediaPreview_feature-L91.png

webUIPreview-mediaPreview_feature-L91.png

webUIPreview-mediaPreview_feature-L98.png

webUIPreview-mediaPreview_feature-L98.png

@kulmann kulmann changed the title Resource ids in urls [full-ci] Resource ids in urls Oct 6, 2022
@JammingBen
Copy link
Collaborator

JammingBen commented Oct 7, 2022 •
edited by kulmann

Issues I've found:

  • Share a text file via public link -> Open link -> Text editor does not open the contents of the file
  • Share a PDF file via public link -> Open link -> PDF viewer does not open the contents of the file
  • Breadcrumb is broken when having more than 2 levels

@AlexAndBear
Copy link
Contributor

AlexAndBear commented Oct 7, 2022 •
edited by kulmann

Issues I've found:

  • Clicking in on rename action in the context actions besides the breadcrumbs on a received share will show the original share name in the rename modal despite the fact, that the share receiver has renamed the share.
    This seems have to do something with the unified folder loader and isn't the case on master.

@kulmann
Copy link
Member Author

kulmann commented Oct 7, 2022

  • Breadcrumb is broken when having more than 2 levels

Mitigated in a recent commit by omitting the fileId of the current folder from the target route query (it was there because we copy the query of the current route to the target route for good reasons).
Obviously this disables navigation by fileId for breadcrumbs on parent folders. For that we'll need propfinds on each parent. Out of scope for this PR.

@dschmidt
Copy link
Member

dschmidt commented Oct 7, 2022

Sharing a single file via public link is broken because of owncloud/ocis#4758

@kulmann kulmann marked this pull request as ready for review October 7, 2022 14:46
@JammingBen
Copy link
Collaborator

It also fixes #7715 and #7714, right?

@kulmann
Copy link
Member Author

kulmann commented Oct 10, 2022

It also fixes #7715 and #7714, right?

yep

@sonarcloud
Copy link

sonarcloud bot commented Oct 10, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

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

23.6% 23.6% Coverage
0.0% 0.0% Duplication

@kulmann kulmann merged commit cc0eec8 into master Oct 10, 2022
@delete-merged-branch delete-merged-branch bot deleted the resource-ids-in-urls branch October 10, 2022 12:37
ownclouders pushed a commit that referenced this pull request Oct 10, 2022
@kulmann
commit cc0eec8
aa64984 
Author: Benedikt Kulmann <benedikt@kulmann.biz>
Date:   Mon Oct 10 14:37:10 2022 +0200

    [full-ci] Resource ids in urls (#7725)

    Co-authored-by: Jannik Stehle <jannik.stehle@gmail.com>
    Co-authored-by: Dominik Schmidt <dev@dominik-schmidt.de>
    Co-authored-by: Jan <j.ackermann91@gmail.com>
@kulmann kulmann mentioned this pull request Oct 14, 2022
Open
@micbar micbar mentioned this pull request Nov 25, 2022
Closed
73 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dschmidt dschmidt dschmidt left review comments

@lookacat lookacat lookacat left review comments

@JammingBen JammingBen JammingBen approved these changes

@AlexAndBear AlexAndBear Awaiting requested review from AlexAndBear

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
6 participants
@kulmann @ownclouders @JammingBen @AlexAndBear @dschmidt @lookacat