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

Bugfix/32630/get access list use correct array key #36305

Closed

Conversation

ASerbinski
Copy link
Contributor

Summary

Switch to correct array key in ShareByMailProvider/getAccessList -- should be 'mail' not 'public'.

@kesselb
Copy link
Contributor

kesselb commented Jan 23, 2023

Thank you very much 👍

Can you have a look at the failing test:

1) OCA\ShareByMail\Tests\ShareByMailProviderTest::testGetAccessList
Failed asserting that an array has the key 'public'.

/home/runner/work/server/server/apps/sharebymail/tests/ShareByMailProviderTest.php:1089

@szaimen szaimen added the 3. to review Waiting for reviews label Jan 23, 2023
@szaimen szaimen added this to the Nextcloud 26 milestone Jan 23, 2023
@ASerbinski ASerbinski force-pushed the bugfix/32630/getAccessList-use-correct-array-key branch from bc2c24e to b9eec92 Compare January 24, 2023 15:02
@kesselb
Copy link
Contributor

kesselb commented Jan 24, 2023

Thank you 👍

Please revert the changes to composer.lock

@ASerbinski
Copy link
Contributor Author

ASerbinski commented Jan 24, 2023

Hi @kesselb ; that commit seems to have been brought in from master during rebase?

@ASerbinski ASerbinski force-pushed the bugfix/32630/getAccessList-use-correct-array-key branch from b9eec92 to cc60f3b Compare January 24, 2023 15:37
@ASerbinski ASerbinski force-pushed the bugfix/32630/getAccessList-use-correct-array-key branch from cc60f3b to 9e48b82 Compare January 24, 2023 15:39
This was using the array key "public", which collided with the
array key used for share-by-link in DefaultShareProvider. Now
using correct key "mail".

Signed-off-by: Adam Serbinski <[email protected]>
Change array key from public to mail.

Signed-off-by: Adam Serbinski <[email protected]>
@ASerbinski ASerbinski force-pushed the bugfix/32630/getAccessList-use-correct-array-key branch from 9e48b82 to 9a567fe Compare January 24, 2023 15:40
@ASerbinski
Copy link
Contributor Author

@kesselb ; think I cleared it up by rebasing again.

@kesselb
Copy link
Contributor

kesselb commented Jan 25, 2023

Thank you 👍

I read the discussion in #2834 to understand public and mail.
If I understand it correct, public is fine and the documentation is wrong.

} else {
$al[$k] = $al[$k] || $v;
}

The above code will "merge" the values together.
If something is shared by MailShareProvider or LinkShareProvider public should be true.

I believe that's good news for your other pull request.

@ASerbinski
Copy link
Contributor Author

Hi @kesselb ; I believe that the documentation is correct and that the PR #2834 you referenced is from a more primitive state before the 'mail' key was added.

Per @schiessle ; "... To make it more generic we could also add another key 'mail' to the array, which would probably be the better solution."

@kesselb
Copy link
Contributor

kesselb commented Jan 26, 2023

I believe that the documentation is correct and that the PR #2834 you referenced is from a more primitive state before the 'mail' key was added.

The documentation for mail was added with the mentioned pr.

To make it more generic we could also add another key 'mail' to the array, which would probably be the better solution.

I guess that was an idea, but not done.

The takeaway for your pull request to list the mail shares for a node is to keep the current behavior (add public = true when at least one mail share exist) and add the mail attribute to the result.

I'm unsure if we need this pr. There is nothing broken to be fixed apart from the documentation. Sorry, for asking you to create a pull request for this change.

@ChristophWurst ChristophWurst removed their request for review January 26, 2023 10:31
@miaulalala miaulalala removed their request for review January 26, 2023 12:44
@ASerbinski
Copy link
Contributor Author

Hi @kesselb ; I added a commit to #32631 to comply with your directive to both preserve the prior behavior with respect to the 'public' array key, and to use the 'mail' key per proposed new usage.

@ASerbinski ASerbinski closed this Jan 26, 2023
@kesselb kesselb deleted the bugfix/32630/getAccessList-use-correct-array-key branch January 26, 2023 16:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: getAccessList in ShareByMailProvider uses incorrect array key
3 participants