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

WIP new share urls #9221

Closed
wants to merge 39 commits into from
Closed

WIP new share urls #9221

wants to merge 39 commits into from

Conversation

kulmann
Copy link
Member

@kulmann kulmann commented Jun 13, 2023 •
edited by dschmidt

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:

  • shared with me page: use driveAlias and path to build new routes into shares (not share/<shareName> but the drive alias and path from the share owner, e.g. personal/admin/test/test2/test3)
  • in useDriveResolver: load all mount points if a driveAliasAndItem could not be resolved (store action loadMountPoints)
  • when accepting a share on shared with me page: load the corresponding drive and add it to the runtime spaces store module
  • cleanup: get rid of fake share spaces in the code base
  • add cern use case
  • fix breadcrumbs
  • share indicators for re-shares inside shares (needs fix in server?)
  • indirect shares inside shares are not marked accordingly in right sidebar (needs fix in server?)
  • add id based routing
  • In search: shows inaccessible parent folder for shared resources inside another folder
  • which nav item should be active when navigating into another person's personal space? personal or shares ... or? -> shares
  • remove isShareSpaceResource
  • Show urls with missing path sections (shareRoot should not be user visible for owncloud by default)
    • move ancestor metadata to web-pkg/runtime, so it can be used by drive resolver and breadcrumbs (gives us an efficient shared cache)
    • drive resolver should return item without ellipsis symbol, so we always deal with proper paths - visiblePath needs to be handled explicitly in the ui
    • we don't know if a fileId belongs to a file or folder, so we need to make a request anyway: if file, keep resource in ancestor meta data (?) so we don't need to make another request in apps to show resource in app-top-bar. if folder, can we set it as currentFolder already?
    • can replaceInvalidFileRoute be handled in useDriveResolver now?
    • what is left of the FolderLoaderSpace at all after this? Potentially it's merged into (or used by) the useDriveResolver directly
    • can we get rid of app apis? loadFolderForFileContext, replaceInvalidFileRoute`,
    • fix navigation when closing apps
  • hide paths
    • in search
  • check uploads
  • check private links
  • reenable invalid route redirects
  • fix flicker when clicking personal space breadcrumb

Follow up tasks:

  • Make all navigation, file and folder loading purely fileid based. Needs getting rid of owncloud-sdk, porting web-client to axios

potential follow up tasks

  • fix flicker when navigating down
  • cancel still running ancestor requests

Open questions

  • How do we want to display paths with hidden segments in url and breadcrumbs?
    • /personal/einstein/…/foo
    • personal/einstein > … > foo ? What should be shown as root space? We currently only get the driveAlias from the API

@update-docs
Copy link

update-docs bot commented Jun 13, 2023

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.

@sonarcloud
Copy link

sonarcloud bot commented Jun 13, 2023

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 1 Code Smell

68.8% 68.8% Coverage
0.0% 0.0% Duplication

@ownclouders
Copy link
Contributor

Results for e2e-tests oC10 https://drone.owncloud.com/owncloud/web/36511/11/1

💥 To see the trace, please open the link in the console ...

npx playwright show-trace https://cache.owncloud.com/public/owncloud/web/36511/tracing/quick-link-alice-2023-6-13-02-49-42.zip

npx playwright show-trace https://cache.owncloud.com/public/owncloud/web/36511/tracing/quick-link-anonymous-2023-6-13-02-49-51.zip

💥 The e2e-oc10 tests pipeline failed. The build has been cancelled.

dschmidt
dschmidt reviewed Aug 15, 2023
View reviewed changes
packages/web-pkg/src/composables/driveResolver/useDriveResolver.ts
},
{ immediate: true, deep: true }
)
watch(
space,
(s: Resource) => {
if (!s || ['public', 'share', 'personal'].includes(s.driveType)) {
if (!s || ['public', 'share', 'personal', 'mountpoint'].includes(s.driveType)) {
Copy link
Member

Choose a reason for hiding this comment

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

remove share type

@dschmidt
Copy link
Member

@tbsbdr please see open questions

@dschmidt
Copy link
Member

@tbsbdr please see open questions

These are kinda important, let's discuss asap

@JammingBen JammingBen mentioned this pull request Sep 11, 2023
Merged
5 tasks
@dschmidt dschmidt force-pushed the mindblowing-share-urls branch 2 times, most recently from fa973c3 to e066302 Compare September 12, 2023 15:44
kulmann and others added 17 commits September 13, 2023 16:27
dschmidt
dschmidt reviewed Sep 15, 2023
View reviewed changes
packages/web-app-files/src/helpers/breadcrumbs.ts
@@ -55,7 +55,7 @@ export const breadcrumbsFor = ({
query: {
...omit(currentRoute.query, 'page'),
// FIXME: add only when idBased routing is on
...(false && { fileId: currentAncestorMetaDataValue.id })
...(true && { fileId: currentAncestorMetaDataValue.id })
Copy link
Member

Choose a reason for hiding this comment

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

😬

@JammingBen JammingBen mentioned this pull request Sep 21, 2023
Merged
16 tasks
@JammingBen
Copy link
Collaborator

Superseded by #9721

@JammingBen JammingBen closed this Sep 22, 2023
@kulmann kulmann deleted the mindblowing-share-urls branch October 27, 2023 07:14
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

Assignees

@dschmidt dschmidt

Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@kulmann @ownclouders @dschmidt @JammingBen