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] new url format #6137

Merged
merged 10 commits into from Jan 3, 2022
Merged

[full-ci] new url format #6137

merged 10 commits into from Jan 3, 2022

Conversation

fschade
Copy link
Collaborator

@fschade fschade commented Dec 12, 2021

Preface

CI will fail with integration tests, and maybe on e2e tests too.
It is meant as a preview to get test early and see what ci says.

The new route naming (we used string matching to identify routes) must be adopted in vendor applications.

Description

this pr introduces the new URL design, it also touches some parts that are not discussed yet.
The technical design makes all url paths interchangeable without touching the logic.

To get a better background please read #6085 and https://github.com/owncloud/ocis/blob/master/docs/ocis/adr/0011-global-url-format.md.

Following things are included:

  • new url grouping (common, spaces, operations and shares)
  • new files routes and route helpers
  • refactoring of the breadcrumb to not rely on static pathes anymore
  • route active detection now is based on vue router resolve instead of pure string matching
  • preparations for spaces namespace and storage
  • deprecation path for old routes

please read the routes overview section to get a better understanding of which parts have changed.

Related Issue

Motivation and Context

prepare web to support spaces and introduce a clear mental cluster of what route should go where.

How Has This Been Tested?

  • unit tests
  • manual testing

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

Open tasks:

  • implement route url schema
  • enable history mode
  • resource id - path fallback and error handling
  • more manual testing
  • remove dev leftovers

Routes overview

Old schema

name path
files-personal /files/list/all/:item*
files-favorites /files/list/favorites
files-shared-with-me /files/list/shared-with-me
files-shared-with-others /files/list/shared-with-others
files-shared-via-link /files/list/shared-via-link
files-trashbin /files/list/trash-bin
files-public-list /files/public/list/:item*
files-public-link /files/public-link/:token
files-private-link /files/private-link/:fileId
files-location-picker /files/location-picker/:context/:action/:item*
files-public-drop /files/files-drop/:token

New schema

name path
files-spaces-storage /files/spaces/:type/:storage/:item*
files-shares-with-me /files/shares/with-me
files-shares-with-others /files/shares/with-others
files-shares-via-link /files/shares/via-link
files-public-files /files/public/show/:item*
files-public-drop /files/public/drop/:token
files-common-favorites /files/favorites
files-common-trash /files/trash
files-operations-resolver-private-link /files/ops/resolver/private-link/:fileId
files-operations-resolver-public-link /files/ops/resolver/public-link/:token
files-operations-location-picker /files/ops/location-picker/:context/:action/:item*

Detailed design

New schema

/files/spaces

  • name: -
  • example: /files/spaces
  • description: redirects to spaces-storage

/files/spaces/:namespace

  • name: -
  • example: /files/spaces/personal
  • description: redirects to spaces-storage

/files/spaces/:namespace/:storage/:item*

  • name: files-spaces-storage
  • example: /files/spaces/personal/admin/any/file/path
  • description:
    • if namespace is not given it falls back to personal
    • if storage is not given it falls back to current user id
    • for now namespace is hard coded, the implementation is prepared to request it from the server

/files/shares

  • name: -
  • example: /files/shares/public/files/foo/bar/baz
  • redirects: files-shares-with-me

/files/public/show/:item*

  • name: files-public-files
  • example: /files/public/show/foo/bar/baz

/files/public/drop/:token

  • name: files-public-drop
  • example: /files/public/drop/45dfgdf5gs5f4g56gd

/files/shares/with-me

  • name: files-shares-with-me
  • example: /files/shares/with-me

/files/shares/with-others

  • name: files-shares-with-others
  • example: /files/shares/with-others

/files/shares/via-link

  • name: files-shares-via-link
  • example: /files/shares/via-link

/files/trash

  • name: files-common-trash
  • example: /files/trash

/files/favorites

  • name: files-common-favorites
  • example: /files/favorites

/files/ops/location-picker/:context/:action/:item*

  • name: files-common-favorites
  • example: /files/ops/location-picker/private/copy/a/deep/path

/files/ops/resolver/public-link/:token

  • name: files-operations-resolver-public-link
  • example: /files/ops/resolver/public-link/45dfgdf5gs5f4g56gd

/files/ops/resolver/private-link/:fileId

  • name: files-operations-resolver-private-link
  • example: /files/ops/resolver/private-link/45dfgdf5gs5f4g56gd

Old schema

/files/list

  • name: -
  • example: /files/list
  • redirect: spaces-storage (defaults namespace and storage)

/files/list/all/item*

  • name: files-personal
  • example: /files/list/all/a/deep/path
  • redirect: spaces-storage (defaults namespace and storage)

/files/list/favorites

  • name: files-favorites
  • example: /files/list/favorites
  • redirect: files-common-favorites

/files/list/shared-with-me

  • name: files-shared-with-me
  • example: /files/list/shared-with-me
  • redirect: files-shares-with-me

/files/list/shared-with-others

  • name: files-shared-with-others
  • example: /files/list/shared-with-others
  • redirect: files-shares-with-others

/files/list/shared-via-link

  • name: files-shared-via-link
  • example: /files/list/shared-via-link
  • redirect: files-shares-via-link

/files/list/trash-bin

  • name: files-trashbin
  • example: /files/list/trash-bin
  • redirect: files-common-trash

/files/public/list/:item*

  • name: files-public-list
  • example: /files/public/list/:item*
  • redirect: files-public-files

/files/public-link/:token

  • name: files-public-list
  • example: /files/public-link/:token
  • redirect: files-operations-resolver-public-link

/files/private-link/:fileId

  • name: files-private-link
  • example: /files/private-link/:fileId
  • redirect: files-operations-resolver-private-link

/files/location-picker/:context/:action/:item*

  • name: files-location-picker
  • example: /files/location-picker/private/move/foo/
  • redirect: -

/files/files-drop/:token

  • name: files-public-drop
  • example: /files/files-drop/5gfd4s56dfgf56gdf
  • redirect: -

@fschade fschade added Status:Needs-Review Needs review from a maintainer Type:Discussion Category:Change Change existing functionality labels Dec 12, 2021
@fschade fschade self-assigned this Dec 12, 2021
@update-docs
Copy link

update-docs bot commented Dec 12, 2021

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.

@ScharfViktor
Copy link
Contributor

ScharfViktor commented Dec 14, 2021

New schema

I don't know how deep you have to dig here. I checked only opening new url

name path test result
files-spaces-storage /files/spaces/:namespace/:storage/:item* ✔️
files-shares-public-files /files/shares/public/files/:item* ✔️
files-shares-public-link /files/shares/public/link/:token ✔️. ❌#/files/public-link/ifpB942A74lZjRx
files-shares-public-drop /files/shares/public/drop/:token ✔️ ❌/#/files/public-link/eOkmMjffDbsAwiY
files-shares-private-link /files/shares/private/link/:fileId ✔️
files-shares-with-me /files/shares/with-me ✔️
files-shares-with-others /files/shares/with-others ✔️
files-shares-via-link /files/shares/via-link ✔️
files-common-favorites /files/favorites ✔️
files-common-trash /files/trash ✔️
files-operations-location-picker /files/ops/location-picker/:context/:action/:item*

public-link and public-drop require logins

@fschade fschade force-pushed the files-url-format branch 2 times, most recently from 303ea0d to cdb227d Compare December 15, 2021 12:07
@fschade
Copy link
Collaborator Author

fschade commented Dec 15, 2021

@ScharfViktor many thanks for testing and find the broken pathes, i missed a redirect from old to the new route. this is done now and can be re-tested.

@ScharfViktor
Copy link
Contributor

ScharfViktor commented Dec 15, 2021

I tried to run smoke tests:

This needs to change:
await page.click('a[href="#/files/spaces/"]') -> https://github.com/owncloud/web/blob/master/tests/smoke/support/page/files/allFiles.ts#L15

await page.click('a[href="#/files/shares/with-me"]') -> https://github.com/owncloud/web/blob/master/tests/smoke/support/page/files/sharedWithMe.ts#L12

But the tests were failed because of this problem (MOVE or COPY buttons were not found)

Untitled.mov

@fschade fschade force-pushed the files-url-format branch 2 times, most recently from 5b9e373 to c3bb828 Compare December 15, 2021 19:40
@fschade
Copy link
Collaborator Author

fschade commented Dec 15, 2021

I tried to run smoke tests:

This needs to change: await page.click('a[href="#/files/spaces/"]') -> https://github.com/owncloud/web/blob/master/tests/smoke/support/page/files/allFiles.ts#L15

await page.click('a[href="#/files/shares/with-me"]') -> https://github.com/owncloud/web/blob/master/tests/smoke/support/page/files/sharedWithMe.ts#L12

But the tests were failed because of this problem (MOVE or COPY buttons were not found)

Untitled.mov

thank you for digging into it, the move, copy etc. actions where missed due the problem that web right now is not able to wait with rendering till the user is fetched (only right after the first login), that is known and as a temporary workaround i removed the user name from storage (/files/spaces/personal/alice) and use home instead (/files/spaces/personal/home).

Thats not cool but the only workaround i could think of till we update the boot mechanism to support this.

I hope, i catched all other problems too, lets see what ci is saying now :)

@ownclouders
Copy link
Contributor

Results for oC10SharingAccept https://drone.owncloud.com/owncloud/web/21096/15/1
The following scenarios passed on retry:

  • webUISharingAcceptSharesToRoot/acceptShares.feature:108

@fschade fschade force-pushed the files-url-format branch 3 times, most recently from ddc1d8a to 65c2487 Compare December 24, 2021 09:28
@fschade fschade marked this pull request as ready for review December 24, 2021 09:44
Copy link
Contributor

@tbsbdr tbsbdr left a comment

Choose a reason for hiding this comment

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

as discussed: lgtm

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.

Thanks for the heavy lifting and attention to detail. I have some things that we should discuss:

  • the PR lacks a changelog item. As of now it needs to be a change as existing applications need to adapt (also see below)
  • can we have the route names in constants? It feels repetitive that they are all over the place as strings
  • could you think of a mapping of old route names to new route names as a kind of compatibility layer? I think it is needed in the fileActions mixin only (line 89). With that this PR could be an enhancement instead of a change afaict
  • a note: I like that isLocationSpacesActive(this.$router) checks with the default route, but I don't like the readability of it when placed side by side with explicit route checks. I'd be in favour of making the check explicit as well ( files-spaces-storage-personal, I know that we don't have it but the readability would be much better).
  • could you explain/remind me again why the personal files have to be in the spaces namespace? Technically shares are spaces as well and they don't live in the spaces namespace either. From a user perspective I'd expect to have files/personal/home and later on when project spaces are alive files/spaces/marketing and the like. files/spaces/personal/home is confusing to me.
  • files/shares/public/files/uGtt0VAc7hyu9Ts looks repetitive as well. Would be much nicer to have just files/shares/public/uGtt0VAc7hyu9Ts would look better (without the second files segment).

(Some more smaller things in the comments)

packages/web-app-files/src/router/utils.ts Show resolved Hide resolved
packages/web-app-files/src/helpers/route.js Show resolved Hide resolved
packages/web-app-files/src/router/utils.ts Outdated Show resolved Hide resolved
packages/web-app-files/src/index.js Outdated Show resolved Hide resolved
new files routes and route helpers
refactoring of the breadcrumb to not rely on static pathes anymore
route active detection now is based on vue router resolve instead of pure string matching
preparations for spaces namespace and storage
deprecation path for old routes
replace response check with navigation check in smoke tests
add workaround to wait for share to be ready after accepting it
update url schema and extract public urls into own namespace
update route spaces helper to have personal namespace written out
add changelog
@fschade
Copy link
Collaborator Author

fschade commented Jan 1, 2022

Thanks for the heavy lifting and attention to detail. I have some things that we should discuss:

  • the PR lacks a changelog item. As of now it needs to be a change as existing applications need to adapt (also see below)
  • can we have the route names in constants? It feels repetitive that they are all over the place as strings
  • could you think of a mapping of old route names to new route names as a kind of compatibility layer? I think it is needed in the fileActions mixin only (line 89). With that this PR could be an enhancement instead of a change afaict
  • a note: I like that isLocationSpacesActive(this.$router) checks with the default route, but I don't like the readability of it when placed side by side with explicit route checks. I'd be in favour of making the check explicit as well ( files-spaces-storage-personal, I know that we don't have it but the readability would be much better).
  • could you explain/remind me again why the personal files have to be in the spaces namespace? Technically shares are spaces as well and they don't live in the spaces namespace either. From a user perspective I'd expect to have files/personal/home and later on when project spaces are alive files/spaces/marketing and the like. files/spaces/personal/home is confusing to me.
  • files/shares/public/files/uGtt0VAc7hyu9Ts looks repetitive as well. Would be much nicer to have just files/shares/public/uGtt0VAc7hyu9Ts would look better (without the second files segment).

(Some more smaller things in the comments)

@kulmann i hope I haven't missed a point on your list, please have a look. As we had additional feedback which could break things we need to re-run all tests done before. Cc.: @ScharfViktor

@sonarcloud
Copy link

sonarcloud bot commented Jan 2, 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 2 Code Smells

55.6% 55.6% Coverage
0.4% 0.4% Duplication

@ownclouders
Copy link
Contributor

Results for oCISSharingBasic https://drone.owncloud.com/owncloud/web/21475/53/1
The following scenarios passed on retry:

  • webUISharingAcceptShares/acceptShares.feature:94

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.

Hooooray! 🥳 🎈 💯 🥇 Awesome piece of work, thank you @fschade ! ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category:Change Change existing functionality Status:Needs-Review Needs review from a maintainer Type:Discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New URL format
6 participants