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] Simplify link creation in Files/Sidebar #6961

Merged
merged 8 commits into from
Jun 14, 2022
Merged

[full-ci] Simplify link creation in Files/Sidebar #6961

merged 8 commits into from
Jun 14, 2022

Conversation

pascalwengerter
Copy link
Contributor

@pascalwengerter pascalwengerter commented May 12, 2022 •
edited
Loading

Description

This will most likely be a bit of a pain in the acceptance tests, but simplifies link creation by the user and reduces code complexity by quite a bit

It also shifts from using the capability-based defaultLinkName to a translatable default defined in the frontend

Related Issue

@ownclouders
Copy link
Contributor

ownclouders commented May 12, 2022 •
edited
Loading

Results for oC10SharingPublic2 https://drone.owncloud.com/owncloud/web/26190/38/1

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

webUISharingPublicDifferentRoles-shareByPublicLinkDifferentRoles_feature-L276.png

webUISharingPublicDifferentRoles-shareByPublicLinkDifferentRoles_feature-L276.png

This was referenced May 13, 2022
Merged
Closed
@pascalwengerter pascalwengerter force-pushed the simplify-sidebar-link-creation branch 4 times, most recently from 0ddd311 to 7162b72 Compare May 18, 2022 20:22
@pascalwengerter pascalwengerter mentioned this pull request May 19, 2022
Merged
@pascalwengerter pascalwengerter force-pushed the simplify-sidebar-link-creation branch 3 times, most recently from b6de9cc to f25cb13 Compare May 19, 2022 17:39
kulmann
kulmann requested changes May 20, 2022
View reviewed changes
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.

Some weird findings (please check if you can reproduce those before digging into code):

  • using ENTER in password prompt continues with link creation but doesn't close the modal
  • setting password enforcement for read only public links and trying to set a different role on an existing link doesn't work. I get the confirmation toast but the role is not updated.

packages/web-app-files/src/components/SideBar/Shares/FileLinks.vue Outdated Show resolved Hide resolved
packages/web-app-files/src/components/SideBar/Shares/FileLinks.vue Show resolved Hide resolved
packages/web-app-files/src/components/SideBar/Shares/FileLinks.vue Outdated Show resolved Hide resolved
@pascalwengerter pascalwengerter mentioned this pull request May 23, 2022
Closed
@pascalwengerter pascalwengerter force-pushed the simplify-sidebar-link-creation branch 2 times, most recently from f45b27f to e7011b7 Compare May 23, 2022 18:45
@pascalwengerter pascalwengerter mentioned this pull request May 23, 2022
Merged
@pascalwengerter
Copy link
Contributor Author

webUISharingPublicDifferentRoles/shareByPublicLinkDifferentRoles.feature:278 fails in CI, can't reproduce locally. Can someone else take a look?

kulmann
kulmann approved these changes Jun 2, 2022
View reviewed changes
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.

LGTM now 👍

@pascalwengerter
Copy link
Contributor Author

@individual-it could your team take a look?
webUISharingPublicDifferentRoles/shareByPublicLinkDifferentRoles.feature:278 fails reliably in CI (e.g. https://drone.owncloud.com/owncloud/web/25984/38/20, but that was a re-run already), but passes reliably when I run the tests locally. Seems to be a hickup in the middleware

@individual-it
Copy link
Member

@pascalwengerter will do, but most likely we will not be able to do it this week

@pascalwengerter
Copy link
Contributor Author

@pascalwengerter will do, but most likely we will not be able to do it this week

It's not super pressing but of course the earlier we can merge the better. Next week is fine with me!

@grgprarup grgprarup self-assigned this Jun 7, 2022
@pascalwengerter pascalwengerter force-pushed the simplify-sidebar-link-creation branch 2 times, most recently from 7640d0a to f5463e8 Compare June 7, 2022 11:08
@pascalwengerter
Copy link
Contributor Author

Just rebased, only one CI failure which passes reliable locally - restarted CI to see if it's (potentially) flaky

https://drone.owncloud.com/owncloud/web/26061/38/20

@grgprarup
Copy link
Contributor

grgprarup commented Jun 8, 2022 •
edited
Loading

Just rebased, only one CI failure which passes reliable locally - restarted CI to see if it's (potentially) flaky

https://drone.owncloud.com/owncloud/web/26061/38/20

@pascalwengerter I were looking into it. I run it on my local machine, sometimes it gets passed, and sometimes fails. But after using a delay before fetching the Share details it always passes. Needs more investigation.

@grgprarup
Copy link
Contributor

@pascalwengerter are you aware of this message Failed to create link and Link was created successfully when adding a link. And sidebar is not updated. Need to close and reopen sidebar to see link.
failed

@pascalwengerter
Copy link
Contributor Author

@pascalwengerter are you aware of this message Failed to create link and Link was created successfully when adding a link. And sidebar is not updated. Need to close and reopen sidebar to see link. failed

Didn't see this, which backend/setup are you using (e.g. required passwords/expiration dates) and which button did you press (assuming Add link in the sidebar)?

@grgprarup
Copy link
Contributor

@pascalwengerter are you aware of this message Failed to create link and Link was created successfully when adding a link. And sidebar is not updated. Need to close and reopen sidebar to see link. failed

Didn't see this, which backend/setup are you using (e.g. required passwords/expiration dates) and which button did you press (assuming Add link in the sidebar)?

backend: oc10
When Add link was clicked, when Enforce password protection for read-only links was not enabled.
When the password set button was clicked, when Enforce password protection for read-only links was enabled.
Tried with simplify-sidebar-link-creation branch
input_password

@pascalwengerter
Copy link
Contributor Author

backend: oc10 When Add link was clicked, when Enforce password protection for read-only links was not enabled. When the password set button was clicked, when Enforce password protection for read-only links was enabled. Tried with simplify-sidebar-link-creation branch

I don't fully get what you're writing. If you first log in as user and then change the enforcement in the settings, we run into the problem of caches capabilities so this is nothing that needs further investigation (since it's known/broken by design).

My problem is that webUISharingPublicDifferentRoles/shareByPublicLinkDifferentRoles.feature:27{6,8} fails reliably in CI and passes reliably for me when running the dockerized acceptance tests

@pascalwengerter pascalwengerter force-pushed the simplify-sidebar-link-creation branch 2 times, most recently from b8cd299 to caefa69 Compare June 13, 2022 11:43
@individual-it
Copy link
Member

yarn install keeps on failing :-(

@grgprarup grgprarup force-pushed the simplify-sidebar-link-creation branch from ba61f42 to 9833ebc Compare June 14, 2022 08:20
grgprarup
grgprarup reviewed Jun 14, 2022
View reviewed changes
tests/acceptance/pageObjects/FilesPageElement/publicLinksDialog.js
Comment on lines +491 to +494
await this.waitForElementVisible('@dialogConfirmBtnEnabled')
.initAjaxCounters()
.click('@dialogConfirmBtnEnabled')
.waitForOutstandingAjaxCalls()
Copy link
Contributor

Choose a reason for hiding this comment

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

@pascalwengerter I have made these changes. IMO this might fix the webUISharingPublicDifferentRoles/shareByPublicLinkDifferentRoles.feature:27{6,8} failure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good to me, let's restart CI later this day

@sonarcloud
Copy link

sonarcloud bot commented Jun 14, 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 7 Code Smells

28.2% 28.2% Coverage
0.0% 0.0% Duplication

@pascalwengerter pascalwengerter merged commit 6930e9f into master Jun 14, 2022
@delete-merged-branch delete-merged-branch bot deleted the simplify-sidebar-link-creation branch June 14, 2022 11:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@individual-it individual-it individual-it left review comments

@grgprarup grgprarup grgprarup left review comments

@kulmann kulmann kulmann approved these changes

@fschade fschade Awaiting requested review from fschade

Assignees

@grgprarup grgprarup

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

"add link" should directly create a new link
5 participants
@pascalwengerter @ownclouders @individual-it @grgprarup @kulmann