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

[stable10] use a multi line approach to display share autocomplete #35397

Merged
merged 8 commits into from
Jun 14, 2019

Conversation

DeepDiver1975
Copy link
Member

@DeepDiver1975 DeepDiver1975 commented May 31, 2019

Description

Related Issue

How Has This Been Tested?

Screenshots (if appropriate):

Federated or Guest

Screenshot from 2019-05-31 17-26-25

Group

Screenshot from 2019-05-31 17-26-00

With additional user information

Screenshot from 2019-05-31 17-25-46

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Database schema changes (next release will require increase of minor version instead of patch)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

Open tasks:

  • Backport (if applicable set "backport-request" label and remove when the backport was done)

Copy link
Contributor

@pmaier1 pmaier1 left a comment

Choose a reason for hiding this comment

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

This is so beautiful ❤️

@DeepDiver1975 DeepDiver1975 changed the title use a multi line approach to display share autocomplete [stable10] use a multi line approach to display share autocomplete May 31, 2019
@micbar
Copy link
Contributor

micbar commented Jun 2, 2019

@DeepDiver1975 Seems like the WebUI Sharing tests have on failing Scenario

@phil-davis phil-davis self-assigned this Jun 3, 2019
@phil-davis
Copy link
Contributor

With this PR the webUI has a separate UI element that displays the type of the potential share recipient. e.g. "User", "Group", "Federated". Previously this information was just appended in text after the display name "(group)", "(federated)".

The webUI tests need enhancing so that they process this extra UI element when working out which share recipient to click on in the dropdown list, and when checking if the correct matching entries are found in the dropdown list.

I will look.

@codecov
Copy link

codecov bot commented Jun 3, 2019

Codecov Report

Merging #35397 into stable10 will decrease coverage by 0.02%.
The diff coverage is 57.14%.

Impacted file tree graph

@@              Coverage Diff               @@
##             stable10   #35397      +/-   ##
==============================================
- Coverage       64.68%   64.65%   -0.03%     
  Complexity      20103    20103              
==============================================
  Files            1293     1293              
  Lines           76976    76977       +1     
  Branches         1300     1301       +1     
==============================================
- Hits            49793    49771      -22     
- Misses          26799    26821      +22     
- Partials          384      385       +1
Flag Coverage Δ Complexity Δ
#javascript 53.84% <57.14%> (-0.01%) 0 <0> (ø)
#phpunit 65.8% <ø> (-0.04%) 20103 <ø> (ø)
Impacted Files Coverage Δ Complexity Δ
core/js/sharedialogview.js 78.1% <57.14%> (-0.68%) 0 <0> (ø)
apps/files_sharing/lib/External/Manager.php 77.38% <0%> (-12.62%) 32% <0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e301daf...f11dd61. Read the comment docs.

@codecov
Copy link

codecov bot commented Jun 3, 2019

Codecov Report

Merging #35397 into stable10 will increase coverage by <.01%.
The diff coverage is 57.14%.

Impacted file tree graph

@@              Coverage Diff               @@
##             stable10   #35397      +/-   ##
==============================================
+ Coverage        64.8%    64.8%   +<.01%     
  Complexity      20183    20183              
==============================================
  Files            1293     1293              
  Lines           77170    77173       +3     
  Branches         1300     1301       +1     
==============================================
+ Hits            50010    50012       +2     
  Misses          26776    26776              
- Partials          384      385       +1
Flag Coverage Δ Complexity Δ
#javascript 53.85% <57.14%> (ø) 0 <0> (ø) ⬇️
#phpunit 65.96% <ø> (ø) 20183 <ø> (ø) ⬇️
Impacted Files Coverage Δ Complexity Δ
core/js/sharedialogview.js 78.6% <57.14%> (-0.19%) 0 <0> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7d1a344...4644552. Read the comment docs.

@@ -98,6 +118,32 @@ public function userFromServerHasSharedWithUserFromServer(
);
}

/**
* @Given /^user "([^"]*)" from server "(LOCAL|REMOTE)" has shared "([^"]*)" with user "([^"]*)" from server "(LOCAL|REMOTE)" with permissions (.*)$/
Copy link
Contributor

Choose a reason for hiding this comment

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

This is specially useful for setting up a share on the remote (federated) server, and giving it some permissions.
We want to use the API to set up stuff on the remote server, rather than the webUI, because we do not want to depend on particular details of the webUI on the remote server. (The remote server might be running some older release/code that has a bit different webUI behaviour, but it should have compatible API behaviour within the same major version)

Copy link
Contributor

@phil-davis phil-davis Jun 3, 2019

Choose a reason for hiding this comment

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

In the case of this particular PR, the webUI dropdown sharing list has changed its format/text a bit, and so trying to run the new webUI sharing acceptance test code against a remote server without the PR would fail.

@phil-davis phil-davis force-pushed the bugfix/35098 branch 6 times, most recently from 0eff143 to 8f732bf Compare June 7, 2019 09:50
@micbar micbar added this to the development milestone Jun 7, 2019
@micbar
Copy link
Contributor

micbar commented Jun 7, 2019

@phil-davis Great Work! Keep on :-)

@phil-davis phil-davis force-pushed the bugfix/35098 branch 2 times, most recently from 9358885 to c6a148d Compare June 11, 2019 11:04
@phil-davis phil-davis requested a review from individual-it June 11, 2019 11:18
@phil-davis
Copy link
Contributor

phil-davis commented Jun 11, 2019

@individual-it this is ready for review of the acceptance test changes.

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.

tests look good to me

@phil-davis
Copy link
Contributor

@pmaier1 and anyone else who cares, this is "good to go" from a technical POV.
If you are happy with it, then we can make the forward-port to master and merge both PRs.
And I guess it will be released in the next minor release?
(not for 10.2.1 ?)

@phil-davis
Copy link
Contributor

Forward port to master is PR #35486

@pmaier1
Copy link
Contributor

pmaier1 commented Jun 11, 2019

And I guess it will be released in the next minor release?
(not for 10.2.1 ?)

Yes, something for the next minor.

@phil-davis
Copy link
Contributor

See comment on forward port about codecov #35486 (comment)
Needs to be decided before merging here, so that the forward port and this stable10 PR are the same.

@phil-davis
Copy link
Contributor

I cherry-picked commit "add js test for remote share with server" from the master PR #35486

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

Successfully merging this pull request may close these issues.

7 participants