Fixes to share list view panel after expiration refactor by mrow4a · Pull Request #36847 · owncloud/core

mrow4a · 2020-01-28T22:41:46Z

This PR targets two problems listed in #36735:

make sure that share options are displayed when toggled (previously logic was enabled->disabled->enabled or enabled->disabled, now disabled->disabled or disabled->enabled)
when unsetting expiry do not show datepicker

codecov · 2020-01-28T22:57:26Z

Codecov Report

Merging #36847 into master will increase coverage by 0.53%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master   #36847      +/-   ##
============================================
+ Coverage     64.17%   64.70%   +0.53%     
  Complexity    19130    19130              
============================================
  Files          1270     1270              
  Lines         74819    74868      +49     
  Branches       1327     1329       +2     
============================================
+ Hits          48012    48447     +435     
+ Misses        26416    26030     -386     
  Partials        391      391

Flag	Coverage Δ	Complexity Δ
#javascript	`54.17% <56.52%> (+0.07%)`	`0.00 <0.00> (ø)`	⬆️
#phpunit	`65.88% <ø> (+0.59%)`	`19130.00 <ø> (ø)`	⬆️

Impacted Files	Coverage Δ	Complexity Δ
apps/files/lib/Command/Scan.php	`71.92% <0.00%> (-9.75%)`	`61.00% <0.00%> (ø%)`
lib/private/Files/Cache/HomePropagator.php	`77.77% <0.00%> (-9.73%)`	`3.00% <0.00%> (ø%)`
lib/private/DB/QueryBuilder/QueryBuilder.php	`91.34% <0.00%> (+0.48%)`	`68.00% <0.00%> (ø%)`	⬆️
apps/dav/lib/Connector/Sabre/File.php	`84.19% <0.00%> (+0.64%)`	`115.00% <0.00%> (ø%)`	⬆️
lib/private/DB/MDB2SchemaWriter.php	`94.79% <0.00%> (+1.04%)`	`34.00% <0.00%> (ø%)`	⬆️
lib/private/DB/MDB2SchemaManager.php	`91.22% <0.00%> (+1.75%)`	`17.00% <0.00%> (ø%)`	⬆️
lib/private/Files/Cache/Cache.php	`97.64% <0.00%> (+1.76%)`	`106.00% <0.00%> (ø%)`	⬆️
lib/private/AllConfig.php	`95.89% <0.00%> (+2.05%)`	`43.00% <0.00%> (ø%)`	⬆️
lib/private/Share/Share.php	`71.15% <0.00%> (+2.27%)`	`501.00% <0.00%> (ø%)`	⬆️
apps/files_versions/lib/Storage.php	`72.39% <0.00%> (+2.53%)`	`95.00% <0.00%> (ø%)`	⬆️
... and 20 more

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 23d339b...1e516e6. Read the comment docs.

phil-davis · 2020-01-29T04:27:17Z

The "notify by email" button is now only displayed when the share-actions-dropdown is opened. Previously it was always shown.

With it closed, the "notify by email" button is not seen:

With it open, the "notify by email" button is seen:

The acceptance tests were failing, e.g.:
https://drone.owncloud.com/owncloud/core/23008/90/15

--- Failed scenarios:

    /drone/src/tests/acceptance/features/webUISharingInternalUsers/shareWithUsers.feature:278
    /drone/src/tests/acceptance/features/webUISharingInternalUsers/shareWithUsers.feature:293
    /drone/src/tests/acceptance/features/webUISharingInternalUsers/shareWithUsers.feature:308
    /drone/src/tests/acceptance/features/webUISharingInternalUsers/shareWithUsers.feature:672

69 scenarios (65 passed, 4 failed)
725 steps (712 passed, 4 failed, 9 skipped)

That change in behaviour needs a change to the acceptance tests so that they open the ShareActionsDropdown before trying to click the "notify by email" button. I pushed a commit for that.

What do others think about this change in behaviour? (Maybe it is OK/good)

(other stuff seems to work well)

micbar · 2020-01-29T07:33:17Z

@mrow4a @phil-davis I like this behavior.
Thank you for the fix.

@pmaier1 Please give us your opinion.

mrow4a · 2020-01-29T07:57:03Z

@micbar @phil-davis yes, I thought that this was intended that everything with shareOption should be hidden - currently mailNotificationSpinner, sharePermission, advancedSharePermissions, expiration.

@phil-davis which acceptance tests are failing, all seems passing?
EDIT:
@phil-davis I see you added commits, thanks!

davitol · 2020-01-29T08:36:05Z

@mrow4a @phil-davis I like this behavior.

I also like the behavior of the PR

~~Also found this behavior:~~. Fixed with owncloud/richdocuments#313

phil-davis · 2020-01-29T08:59:11Z

@davitol are you running richdocuments from PR owncloud/richdocuments#313 fix_36751 branch?
That should have the latest richdocuments code to work with core.

mrow4a · 2020-01-29T08:59:16Z

@davitol I have no idea why this is possible. I never seen any problems with hovering - in this code there is nothing related to hovering.

I will try to reproduce, might be indeed related to secure view app not aware of expiration

davitol · 2020-01-29T09:18:19Z

@davitol are you running richdocuments from PR owncloud/richdocuments#313 fix_36751 branch?
That should have the latest richdocuments code to work with core.

@phil-davis You were right, with that branch It does not happen. Thanks for the hints

micbar · 2020-01-29T10:37:52Z

@davitol Could you confirm that the sharing bug with secure view is fixed in the PR 303 in richdocuments?

phil-davis · 2020-01-29T11:02:41Z

@davitol Could you confirm that the sharing bug with secure view is fixed in the PR 303 in richdocuments?

PR owncloud/richdocuments#313

phil-davis

works for me
a JS person needs to review the code

pmaier1 · 2020-01-29T11:23:54Z

@pmaier1 Please give us your opinion.

Thanks guys 👍, I agree. What I see in #36847 (comment) looks good now.

If that sharing bug is fixed as well, perfect 👌

davitol · 2020-01-29T11:25:59Z

Should it be possible to have active secure view + can share option enable at the same time?

If that sharing bug is fixed as well, perfect 👌

Current behavior => It is not possible to have enabled at the same time secure view + can share option. @pmaier1 If we agree about this behavior then the bug is fixed

phil-davis · 2020-01-29T11:33:43Z

Should it be possible to have active secure view + can share option enable at the same time?

There was an issue with discussion about that somewhere! It was decided that a resource shared as "Secure View" is not allowed to be re-shared.

If someone can find the link, then post it.

davitol · 2020-01-29T11:36:13Z

If someone can find the link, then post it.

#36751 (comment). ?

PVince81

some dubious logic to be confirmed

core/js/sharedialogshareelistview.js

PVince81 · 2020-01-29T14:58:45Z

core/js/sharedialogshareelistview.js

+				// use mutation observer to ensure that if sharewithlist changes
+				// proper share details are toggled
+				var shareWithList = this;
+				if(_.isUndefined(self._toggleMutationObserver)) {


this block doesn't seem to be related at all with the shareWithList, maybe can move it outside ? or do we need to first make sure that there's at least one element ? if yes, you could have used .at(0) for that instead of the loop

seems the whole logic here can only work if there's only a single shareWithList element, else stuff would overwrite itself here.

for readability, this each() handler should be moved out

PVince81 · 2020-01-29T15:01:40Z

core/js/sharedialogshareelistview.js

+			this.$el.find('li').each(function() {
+				var $li = $(this);
+				var shareId = $li.data('share-id');
+				if (!_.isUndefined(view._currentlyToggled[shareId])) {


this check disregards whether view._currentlyToggled[sharedId] is true or false as it only checks whether the value is defined ("false" counts as defined) and would always set it to true, is that intended ?

good point, clarified the logic

mrow4a · 2020-01-30T19:40:49Z

@davitol @phil-davis @micbar @pmaier1 I solved both issues in core, preserving compatibility with already released versions of richdocuments/onlyoffice. It would be however good that customers update their onlyoffice/richdocuments apps just to be on a safe side for the future in case some other field gets added.

phil-davis · 2020-01-31T01:19:10Z

Acceptance test fails:

--- Failed scenarios:

    /drone/src/tests/acceptance/features/webUISharingInternalUsers/shareWithUsers.feature:278
    /drone/src/tests/acceptance/features/webUISharingInternalUsers/shareWithUsers.feature:293
    /drone/src/tests/acceptance/features/webUISharingInternalUsers/shareWithUsers.feature:308
    /drone/src/tests/acceptance/features/webUISharingInternalUsers/shareWithUsers.feature:672

69 scenarios (65 passed, 4 failed)
725 steps (712 passed, 4 failed, 9 skipped)

https://drone.owncloud.com/owncloud/core/23054/90/15

--- Failed scenarios:

    /drone/src/tests/acceptance/features/webUISharingInternalGroups/shareWithGroups.feature:136
    /drone/src/tests/acceptance/features/webUISharingInternalGroups/shareWithGroups.feature:153
    /drone/src/tests/acceptance/features/webUISharingInternalGroups/shareWithGroups.feature:172
    /drone/src/tests/acceptance/features/webUISharingInternalGroups/shareWithGroups.feature:185

57 scenarios (53 passed, 4 failed)
815 steps (798 passed, 4 failed, 13 skipped)

https://drone.owncloud.com/owncloud/core/23054/89/15

It looks like the "notify by email" button has changed its behaviour again. I will look.

…y email' button

phil-davis · 2020-01-31T04:26:53Z

I pushed my acceptance tests commit back on this PR. It got lost. IMO that will allow CI to pass.

mrow4a · 2020-01-31T07:09:50Z

@phil-davis sorry I probably force pushed to branch and deleted a commit by mistake. Missed message that you pushed something

phil-davis · 2020-01-31T07:13:42Z

https://drone.owncloud.com/owncloud/core/23057/30/6
one of the drone agents had hung - 3 hours in yarn-install step!
I restarted drone.

PVince81 · 2020-01-31T07:40:37Z

core/js/sharedialogshareelistview.js

+			this.$el.find('li').each(function() {
+				var $li = $(this);
+				var shareId = $li.data('share-id');
+				if (!_.isUndefined(view._currentlyToggled[shareId]) && view._currentlyToggled[shareId] === true) {


!isUndefined is now redundant because undefined is not strictly equal to true.
but ok to keep

PVince81

👍

mrow4a requested review from PVince81, lordelix, micbar and phil-davis January 28, 2020 22:41

mrow4a self-assigned this Jan 28, 2020

mrow4a mentioned this pull request Jan 28, 2020

Cross deletion of 'expiration date for user' shows the calendar using Safari as browser #36735

Closed

mrow4a force-pushed the fix_share_view_list_hidding branch from 17e401d to 69e66a5 Compare January 28, 2020 22:57

owncloud deleted a comment from update-docs bot Jan 29, 2020

phil-davis reviewed Jan 29, 2020

View reviewed changes

PVince81 reviewed Jan 29, 2020

View reviewed changes

mrow4a force-pushed the fix_share_view_list_hidding branch from 16cb9fb to 656617d Compare January 30, 2020 19:34

phil-davis mentioned this pull request Jan 31, 2020

Do not update permissions or attributes if other share fields gets updated owncloud/richdocuments#313

Merged

1 task

mrow4a added 2 commits January 31, 2020 09:35

make sure that share options are displayed when toggled

71de2fb

when unsetting expiry do not show datepicker

0f11416

mrow4a and others added 2 commits January 31, 2020 09:35

make sure click on expiry does not reset permissions or attributes

03f7dce

Adjust tests to open ShareActionsDropDown to be able to see 'notify b…

1e516e6

…y email' button

phil-davis force-pushed the fix_share_view_list_hidding branch from 60db761 to 1e516e6 Compare January 31, 2020 03:50

PVince81 reviewed Jan 31, 2020

View reviewed changes

PVince81 approved these changes Jan 31, 2020

View reviewed changes

micbar merged commit 165ef80 into master Jan 31, 2020

delete-merged-branch bot deleted the fix_share_view_list_hidding branch January 31, 2020 08:28

Conversation

mrow4a commented Jan 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Jan 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

phil-davis commented Jan 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

micbar commented Jan 29, 2020

Uh oh!

mrow4a commented Jan 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davitol commented Jan 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

phil-davis commented Jan 29, 2020

Uh oh!

mrow4a commented Jan 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davitol commented Jan 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

micbar commented Jan 29, 2020

Uh oh!

phil-davis commented Jan 29, 2020

Uh oh!

phil-davis left a comment

Choose a reason for hiding this comment

Uh oh!

pmaier1 commented Jan 29, 2020

Uh oh!

davitol commented Jan 29, 2020

Uh oh!

phil-davis commented Jan 29, 2020

Uh oh!

davitol commented Jan 29, 2020

Uh oh!

PVince81 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

PVince81 Jan 29, 2020

Choose a reason for hiding this comment

Uh oh!

mrow4a Jan 30, 2020

Choose a reason for hiding this comment

Uh oh!

PVince81 Jan 29, 2020

Choose a reason for hiding this comment

Uh oh!

mrow4a Jan 30, 2020

Choose a reason for hiding this comment

Uh oh!

mrow4a commented Jan 30, 2020

Uh oh!

phil-davis commented Jan 31, 2020

Uh oh!

phil-davis commented Jan 31, 2020

Uh oh!

mrow4a commented Jan 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

phil-davis commented Jan 31, 2020

Uh oh!

PVince81 Jan 31, 2020

Choose a reason for hiding this comment

Uh oh!

PVince81 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

mrow4a commented Jan 28, 2020 •

edited

Loading

codecov bot commented Jan 28, 2020 •

edited

Loading

phil-davis commented Jan 29, 2020 •

edited

Loading

mrow4a commented Jan 29, 2020 •

edited

Loading

davitol commented Jan 29, 2020 •

edited

Loading

mrow4a commented Jan 29, 2020 •

edited

Loading

davitol commented Jan 29, 2020 •

edited

Loading

mrow4a commented Jan 31, 2020 •

edited

Loading