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] ADD VUE history mode #6363

Merged
merged 8 commits into from
Feb 22, 2022
Merged

[full-ci] ADD VUE history mode #6363

merged 8 commits into from
Feb 22, 2022

Conversation

fschade
Copy link
Collaborator

@fschade fschade commented Feb 3, 2022

Description

Before continue reading please check out #6277 first.

We've decided that we need cleaner URL's and the hash http://foo.tld/#foo/bar shouldn't be there.
To get rid of it we've added an option to enable vue's history mode by using a base tag <base href="PATH">.

Whenever index.html, oidc-callback.html or oidc-silent-redirect.html contains it, we instruct the vue router to use the history mode.

We cannot force enable it by now because:

  • ocis uses history
  • oc10 uses hash
  • sidecar uses hash or history

Also having it as an config option is problematic because we cannot find out (for now) where web looks up for the configuration.json when displaying https://oc.tld/custom/ROOT/some/webdav/PATH.

Using a base tag to enable it, gives us the ability to have information for the vue router which router.base and which router.mode to use.

To use it just add a base tag to index.html, oidc-callback.html, oidc-silent-redirect.html and then enable try_files for your webserver. This is done by default when using ocis and no manual editing is needed once owncloud/ocis#3109 is merged.

Related Issue

Motivation and Context

nice urls

How Has This Been Tested?

  • unit tests

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:

@ownclouders
Copy link
Contributor

Results for oCISSharingInternal3 https://drone.owncloud.com/owncloud/web/22425/61/1
The following scenarios passed on retry:

  • webUISharingInternalUsersSharingIndicator/shareWithUsers.feature:17

@ownclouders
Copy link
Contributor

Results for oC10iPhone1 https://drone.owncloud.com/owncloud/web/22431/48/1
💥 The acceptance tests pipeline failed. The build has been cancelled.

@ownclouders
Copy link
Contributor

Results for oCISFiles3 https://drone.owncloud.com/owncloud/web/22431/57/1
💥 The acceptance tests pipeline failed. The build has been cancelled.

@ownclouders
Copy link
Contributor

Results for oCISSharingInternal1 https://drone.owncloud.com/owncloud/web/22431/59/1
💥 The acceptance tests pipeline failed. The build has been cancelled.

@ownclouders
Copy link
Contributor

Results for oCISSharingBasic https://drone.owncloud.com/owncloud/web/22431/53/1
💥 The acceptance tests pipeline failed. The build has been cancelled.

@ownclouders
Copy link
Contributor

Results for oCISSharingAutocompletion https://drone.owncloud.com/owncloud/web/22431/62/1
💥 The acceptance tests pipeline failed. The build has been cancelled.

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.

Needs documentation about how to activate the history mode, will open an issue for documentation. Changes look good to me! 💪

@ownclouders
Copy link
Contributor

Results for oC10iPhoneNotifications https://drone.owncloud.com/owncloud/web/22861/47/1
💥 The acceptance tests pipeline failed. The build has been cancelled.

@ownclouders
Copy link
Contributor

Results for oC10XGAPortraitNotifications https://drone.owncloud.com/owncloud/web/22861/43/1
💥 The acceptance tests pipeline failed. The build has been cancelled.

@kulmann
Copy link
Member

kulmann commented Feb 17, 2022

In general this PR is good to go. But there is an issue in playwright vs. nightwatch tests: I don't know the reason, but with this PR the playwright (e2e) tests for ownCloud 10 stop with a timeout on the Authorize oauth2 page. That's why I created 5a43820 to set web as trusted client. For the playwright tests this works. But the nightwatch based test suite has some tests that wait for the authorization page of the oauth2 app.

@phil-davis @individual-it do you have an opinion on this? I didn't find out why the playwright tests now timeout on the authorize page. PR diff doesn't give a hint for me. Could you invest time with high prio to unblock this PR? I'm fine with setting web as trusted client and removing the respective tests for the authorize page in the acceptance test suite. That somehow feels like testing vendor code anyway (checking the fact that the oauth2 page redirects to web after clicking authorize is indirectly testing the oauth2 app instead of web).

@kiranparajuli589
Copy link
Contributor

@saw-jan will be investigating the above-mentioned case.

@ownclouders
Copy link
Contributor

Results for oC10Notification https://drone.owncloud.com/owncloud/web/22879/40/1

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

webUINotifications-displayNotificationsOnWebUI_feature-L13.png

webUINotifications-displayNotificationsOnWebUI_feature-L13.png

webUINotifications-displayNotificationsOnWebUI_feature-L18.png

webUINotifications-displayNotificationsOnWebUI_feature-L18.png

💥 The acceptance tests pipeline failed. The build has been cancelled.

@kulmann
Copy link
Member

kulmann commented Feb 18, 2022

Do you think it makes sense to make both test suites able to deal with an existing or absent oauth2 authorize page? Meaning: Shortly waiting if it's there, if yes, click authorize, if not, already done with the step? This would still mean that we need to get rid of scenarios or steps in scenarios where we actively wait for the authorize page to appear. How much effort would that be?

@ownclouders
Copy link
Contributor

Results for oCISBasic https://drone.owncloud.com/owncloud/web/22881/51/1
💥 The acceptance tests pipeline failed. The build has been cancelled.

@saw-jan
Copy link
Member

saw-jan commented Feb 18, 2022

Do you think it makes sense to make both test suites able to deal with an existing or absent oauth2 authorize page? Meaning: Shortly waiting if it's there, if yes, click authorize, if not, already done with the step? This would still mean that we need to get rid of scenarios or steps in scenarios where we actively wait for the authorize page to appear. How much effort would that be?

That approach can be implemented but the problem would the wait time.
Since we need to login on every tests, wait time might affect the test run time cumulatively (by how much extent is unknown)

@ownclouders
Copy link
Contributor

Results for oC10Notification https://drone.owncloud.com/owncloud/web/22882/40/1

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

webUINotifications-displayNotificationsOnWebUI_feature-L13.png

webUINotifications-displayNotificationsOnWebUI_feature-L13.png

webUINotifications-displayNotificationsOnWebUI_feature-L18.png

webUINotifications-displayNotificationsOnWebUI_feature-L18.png

💥 The acceptance tests pipeline failed. The build has been cancelled.

@ownclouders
Copy link
Contributor

Results for oC10XGAPortrait2 https://drone.owncloud.com/owncloud/web/22882/45/1
💥 The acceptance tests pipeline failed. The build has been cancelled.

@saw-jan
Copy link
Member

saw-jan commented Feb 18, 2022

@kulmann , After logging in if we do not wait for some seconds then the login page reappears instead of files page.
And waiting for some seconds after submitting a login form works.
This happens in the tests but cannot reproduce manually. Do you have any idea why that happens?

@ownclouders
Copy link
Contributor

Results for oCISTrashbinUploadMoveJourney https://drone.owncloud.com/owncloud/web/22948/69/1
The following scenarios passed on retry:

  • webUIUserJourney/journey1.feature:11

@ownclouders
Copy link
Contributor

Results for oCISSharingInternal2 https://drone.owncloud.com/owncloud/web/22948/60/1

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

webUISharingInternalUsers-shareWithUsers_feature-L158.png

webUISharingInternalUsers-shareWithUsers_feature-L158.png

@ownclouders
Copy link
Contributor

Results for oCISFiles1 https://drone.owncloud.com/owncloud/web/22948/55/1
The following scenarios passed on retry:

  • webUIDeleteFilesFolders/deleteFilesFolders.feature:12

@ownclouders
Copy link
Contributor

Results for oC10Files1 https://drone.owncloud.com/owncloud/web/22948/16/1

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

webUIFilesCopy-copy_feature-L12.png

webUIFilesCopy-copy_feature-L12.png

webUIFilesCopy-copy_feature-L38.png

webUIFilesCopy-copy_feature-L38.png

@ownclouders
Copy link
Contributor

Results for oCISFiles2 https://drone.owncloud.com/owncloud/web/22948/56/1

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

webUIFilesSearch-search_feature-L113.png

webUIFilesSearch-search_feature-L113.png

webUIFilesSearch-search_feature-L139.png

webUIFilesSearch-search_feature-L139.png

webUIFilesSearch-search_feature-L146.png

webUIFilesSearch-search_feature-L146.png

webUIFilesSearch-search_feature-L185.png

webUIFilesSearch-search_feature-L185.png

webUIFilesSearch-search_feature-L27.png

webUIFilesSearch-search_feature-L27.png

webUIFilesSearch-search_feature-L38.png

webUIFilesSearch-search_feature-L38.png

webUIFilesSearch-search_feature-L95.png

webUIFilesSearch-search_feature-L95.png

@ownclouders
Copy link
Contributor

Results for oC10IntegrationApp1 https://drone.owncloud.com/owncloud/web/22948/71/1

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

webUIFilesSearch-search_feature-L27.png

webUIFilesSearch-search_feature-L27.png

@ownclouders
Copy link
Contributor

Results for oC10Files2 https://drone.owncloud.com/owncloud/web/22948/17/1

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

webUIFilesSearch-search_feature-L103.png

webUIFilesSearch-search_feature-L103.png

webUIFilesSearch-search_feature-L113.png

webUIFilesSearch-search_feature-L113.png

webUIFilesSearch-search_feature-L122.png

webUIFilesSearch-search_feature-L122.png

webUIFilesSearch-search_feature-L129.png

webUIFilesSearch-search_feature-L129.png

webUIFilesSearch-search_feature-L139.png

webUIFilesSearch-search_feature-L139.png

webUIFilesSearch-search_feature-L146.png

webUIFilesSearch-search_feature-L146.png

webUIFilesSearch-search_feature-L153.png

webUIFilesSearch-search_feature-L153.png

webUIFilesSearch-search_feature-L165.png

webUIFilesSearch-search_feature-L165.png

webUIFilesSearch-search_feature-L175.png

webUIFilesSearch-search_feature-L175.png

webUIFilesSearch-search_feature-L185.png

webUIFilesSearch-search_feature-L185.png

webUIFilesSearch-search_feature-L27.png

webUIFilesSearch-search_feature-L27.png

webUIFilesSearch-search_feature-L38.png

webUIFilesSearch-search_feature-L38.png

webUIFilesSearch-search_feature-L50.png

webUIFilesSearch-search_feature-L50.png

webUIFilesSearch-search_feature-L95.png

webUIFilesSearch-search_feature-L95.png

@ownclouders
Copy link
Contributor

Results for oC10Notification https://drone.owncloud.com/owncloud/web/22951/40/1

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

webUINotifications-displayNotificationsOnWebUI_feature-L13.png

webUINotifications-displayNotificationsOnWebUI_feature-L13.png

webUINotifications-displayNotificationsOnWebUI_feature-L18.png

webUINotifications-displayNotificationsOnWebUI_feature-L18.png

@saw-jan
Copy link
Member

saw-jan commented Feb 22, 2022

@fschade @kulmann The real issue was that after the login, the page gets refreshed too early in the tests (which might have prevented some storage or cookies to be set thus causing redirect to login page).
I will be pushing a possible fix.

But it is still a mystery why that happens only in that docker setup (web served via apache and service name used as URL e.g. http://web)

@sonarcloud
Copy link

sonarcloud bot commented Feb 22, 2022

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 0 Code Smells

82.5% 82.5% Coverage
0.0% 0.0% Duplication

@ownclouders
Copy link
Contributor

Results for oCISSharingPublic1 https://drone.owncloud.com/owncloud/web/22955/67/1
The following scenarios passed on retry:

  • webUISharingPublicBasic/publicLinkCreate.feature:190

@fschade
Copy link
Collaborator Author

fschade commented Feb 22, 2022

@saw-jan many thanks 😗

@fschade fschade merged commit 7ced02b into master Feb 22, 2022
@delete-merged-branch delete-merged-branch bot deleted the router-history-mode branch February 22, 2022 15:11
@diocas
Copy link
Contributor

diocas commented Feb 22, 2022

@fschade did you see my comments above?

@fschade
Copy link
Collaborator Author

fschade commented Feb 22, 2022

@diocas

Instead of , you want or am I missing something?;

there's no need to close the base tag https://www.w3schools.com/tags/tag_base.asp#:~:text=The%20tag%20specifies%20the,inside%20the%20element.

I had previously written in here that js and css had relative paths, but it looks like it's just css. If I do not start in / (i.e go to cbox.cern.ch/some/page), the css never gets loaded)

thanks, i create a separate PR for this, the problem we have right now is that we first have to add the base tag to oc10 because it's located in /index.php/apps/web/index.html and having /css/app.css without the base tag could break it.

If I land in a route that doesn't exist, I only see a white page (with the branding header, if the css loads) and maybe it should be a 404;

Yes 404 must be handled inside web (vue) when using the history mode, this is because of the try_file pattern. We need to add those pages in a separate PR

The login page is appearing with the header instead of just being a simple page.. But this might've been some issue when I rebased our changes;

please keep my in the loop if theres something we can do.

@diocas
Copy link
Contributor

diocas commented Feb 22, 2022

the problem was not closing the tag, it's that in ocis (and I think your code) it uses href but you document src (including the changelog), which when I tried was breaking all the redirections. (edit: I've just read the page you sent, and it needs to be href)
For the others, yes, I'll be waiting for the PRs (including the 404 page, which was exactly what I wanted :) ).
Do you have an estimate btw? To know if I need to remove the functionality for now or if the fixes will come soon enough.

@fschade
Copy link
Collaborator Author

fschade commented Feb 22, 2022

@diocas not now, in my opinion we should tackle those fast (i can't decide exactly by when). Now i first need to work on #6327 and #5863

@diocas
Copy link
Contributor

diocas commented Feb 22, 2022

ok, thanks. In the meantime I did a quick fix for the most visible issue (the css not loading), so I think we're good. I'll check the login page with the header issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

URL - VUE router history mode (oCIS only)
7 participants