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

Selecting breadcrumb and reloading inside problematic folder #2530

Merged
merged 1 commit into from
Dec 4, 2019

Conversation

haribhandari07
Copy link
Contributor

@haribhandari07 haribhandari07 commented Nov 22, 2019

Description

Browsing inside problematic folder that contains %2F for eg folder%2Fwith%2FSlashes and then clicking on breadcrumb displays error message. This PR verifies this issue: #1883

Related Issue

#1883

Motivation and Context

How Has This Been Tested?

Manual
CI

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:

@haribhandari07 haribhandari07 marked this pull request as ready for review November 22, 2019 09:32
@haribhandari07 haribhandari07 self-assigned this Nov 22, 2019
@ownclouders
Copy link
Contributor

💥 Acceptance tests Files failed. Please find the screenshots inside ...

https://drone.owncloud.com/owncloud/phoenix/6679/

20191122-094946-675.png
20191122-095028-349.png

Copy link
Contributor

@dpakach dpakach left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@ownclouders
Copy link
Contributor

💥 Acceptance tests Files failed. Please find the screenshots inside ...

https://drone.owncloud.com/owncloud/phoenix/6697/

20191122-115146-835.png
20191122-115231-305.png

navigateToBreadcrumb: async function (resource) {
const resourceXpath = util.format(this.elements.resourceBreadcrumb.selector, resource)
return this
.useXpath()
Copy link
Member

@skshetry skshetry Nov 25, 2019

Choose a reason for hiding this comment

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

Suggested change
.useXpath()
.useStrategy(this.elements.resourceBreadcrumb)

*
* @param {string} resource
*/
navigateToBreadcrumb: async function (resource) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
navigateToBreadcrumb: async function (resource) {
navigateToBreadcrumb: function (resource) {

skshetry
skshetry previously approved these changes Nov 25, 2019
Copy link
Member

@skshetry skshetry left a comment

Choose a reason for hiding this comment

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

Looks good. Please squash all the commits.

@ownclouders
Copy link
Contributor

💥 Acceptance tests Files failed. Please find the screenshots inside ...

https://drone.owncloud.com/owncloud/phoenix/6723/

20191125-060437-334.png
20191125-060517-647.png

@haribhandari07
Copy link
Contributor Author

haribhandari07 commented Nov 27, 2019

rootFolder: "" is set in drone, so breadcrumb is not seen one level below the root folder. So, this PR is currently blocked and will be available after PR #2540 is merged.

@PVince81
Copy link
Contributor

looks like #2540 was merged already

@ownclouders
Copy link
Contributor

💥 Acceptance tests Files failed. Please find the screenshots inside ...

https://drone.owncloud.com/owncloud/phoenix/6908/

20191202-055307-913.png
20191202-055343-193.png

@@ -8,7 +8,7 @@ Feature: access breadcrumb

@issue-2538
Scenario: Check breadCrumb for folder one level below the root folder when rootFolder is set
Given the rootFolder has been set to "" in phoenix config file
Given the "rootFolder" has been set to "" in phoenix config file
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Given the "rootFolder" has been set to "" in phoenix config file
Given the property "rootFolder" has been set to "" in phoenix config file

I think this sounds better.

Copy link
Contributor

@dpakach dpakach left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@ownclouders
Copy link
Contributor

💥 Acceptance tests Files failed. Please find the screenshots inside ...

https://drone.owncloud.com/owncloud/phoenix/6917/

20191202-092504-858.png
20191202-092527-429.png

@ownclouders
Copy link
Contributor

💥 Acceptance tests Trashbin failed. Please find the screenshots inside ...

https://drone.owncloud.com/owncloud/phoenix/6931/

20191202-104339-114.png
20191202-104444-936.png

Copy link
Member

@individual-it individual-it left a comment

Choose a reason for hiding this comment

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

small change request

@PVince81 in #2540 the test were focused on the rootFolder setting, here its more about the special character %2F in the folder name

When the user opens folder "folder%2Fwith%2FSlashes" using the webUI
And the user browses to folder "folder%2Fwith%2FSlashes" using the breadcrumb on the webUI
Then the error message "Loading folder failed…" should be displayed on the webUI
# Then the error message should not be displayed on the webUI
Copy link
Member

Choose a reason for hiding this comment

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

we already have a step for that

Suggested change
# Then the error message should not be displayed on the webUI
# Then no message should be displayed on the webUI

@haribhandari07 haribhandari07 merged commit 5fcb379 into master Dec 4, 2019
@delete-merged-branch delete-merged-branch bot deleted the click-breadcrumbs branch December 4, 2019 08:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status:Needs-Review Needs review from a maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants