Skip to content

fix display of remote users in incoming share notifications - #22062

Merged
blizzz merged 5 commits into
masterfrom
fix/noid/displayname-in-incoming-remote-share-notification
Aug 4, 2020
Merged

fix display of remote users in incoming share notifications#22062
blizzz merged 5 commits into
masterfrom
fix/noid/displayname-in-incoming-remote-share-notification

Conversation

@blizzz

@blizzzblizzz commented Jul 30, 2020•
edited
Loading

Copy link
Copy Markdown
Member
  1. Have two Nextcloud instances with LDAP and User IDs from UUIDs
  2. Make those instances trusted and ensure that addressbooks are exchanged
  3. Do a remote share from one instance to the other
  4. As recipient, have a look at your notifications

Before

Screenshot_20200730_211335

After:

Screenshot_20200730_211355

Hint for reviewers: have a look at the first commit, the second contains code style changes only.

blizzz added 2 commits July 30, 2020 21:10
@blizzz
fix check for Cloud ID, missing return statements, wrong param use
b79e34c
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@blizzz
coding style fixes
04521c6
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@blizzzblizzz added this to the Nextcloud 20 milestone Jul 30, 2020
@blizzz

blizzz commented Jul 30, 2020

Copy link
Copy Markdown
MemberAuthor

/backport to stable19

nickvergessen
nickvergessen reviewed Jul 30, 2020
View reviewed changes
Comment threadapps/federatedfilesharing/lib/Notifier.php
}

try {
return $this->getDisplayNameFromContact($user . '@https://' . $server);

@nickvergessennickvergessenJul 30, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

https first? :)

@blizzzblizzzJul 31, 2020

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

didn't touch the order. can switch. actually i do wonder whether there is a valid use case for those two fall backs?

nickvergessen
nickvergessen reviewed Jul 30, 2020
View reviewed changes
Comment threadapps/dav/lib/CardDAV/CardDavBackend.php

if ($property === 'CLOUD' && preg_match('/[^a-zA-Z0-9 _.@\-\']/', $pattern) === 1) {
// There can be no chars in cloud ids which are not valid for user ids
if ($property === 'CLOUD' && preg_match('/[^a-zA-Z0-9 :_.@\/\-\']/', $pattern) === 1) {

@nickvergessennickvergessenJul 30, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

D'oh!

nickvergessen
nickvergessen reviewed Jul 30, 2020
View reviewed changes
Comment threadapps/dav/lib/CardDAV/CardDavBackend.php Outdated
nickvergessen
nickvergessen reviewed Jul 30, 2020
View reviewed changes
Comment threadapps/dav/lib/CardDAV/CardDavBackend.php Outdated
nickvergessen
nickvergessen reviewed Jul 30, 2020
View reviewed changes
Comment threadapps/dav/lib/CardDAV/CardDavBackend.php Outdated
nickvergessen
nickvergessen reviewed Jul 30, 2020
View reviewed changes
Comment threadapps/dav/lib/CardDAV/CardDavBackend.php Outdated
nickvergessen
nickvergessen reviewed Jul 30, 2020
View reviewed changes
Comment threadapps/dav/lib/CardDAV/CardDavBackend.php Outdated
nickvergessen
nickvergessen reviewed Jul 30, 2020
View reviewed changes
Comment threadapps/federatedfilesharing/lib/Notifier.php
);

$initiator = $params[0];
$initiatorDisplay = isset($params[3]) ? $params[3] : null;

@nickvergessennickvergessenJul 30, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

then we can revert the change where this was added?

@blizzzblizzzJul 31, 2020

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

some details (formating, error information) are still in place.

blizzzand others added 3 commits August 3, 2020 11:05
@blizzz
effectively revert17fae47fixing i10n
ac3d8d1
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@nickvergessen
Fix missing placeholders in translations
79027c8
Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen
Short php syntax
a0ee4b5
Signed-off-by: Joas Schilling <coding@schilljs.com>
@faily-bot

This comment has been minimized.

Sign in to view
@nickvergessennickvergessen mentioned this pull request Aug 3, 2020
Closed
@blizzz
blizzz merged commit 7d2f5af into masterAug 4, 2020
@blizzz
blizzz deleted the fix/noid/displayname-in-incoming-remote-share-notification branch August 4, 2020 16:26
@backportbot-nextcloudbackportbot-nextcloudBot mentioned this pull request Aug 4, 2020
Closed
@blizzzblizzz mentioned this pull request Aug 6, 2020
Merged
@juliusknorr

juliusknorr commented Oct 16, 2020

Copy link
Copy Markdown
Member

Any reason not to backport this to stable18 ?

@blizzz

blizzz commented Oct 16, 2020

Copy link
Copy Markdown
MemberAuthor

Any reason not to backport this to stable18 ?

I don't remember if it was affected or not. If it is, and the fix applies, then nothing speaks against a backport.

@ChristophWurst

ChristophWurst commented Oct 19, 2020

Copy link
Copy Markdown
Member

/backport to stable18

@backportbot-nextcloudbackportbot-nextcloudBot mentioned this pull request Oct 19, 2020
Merged
@ChristophWurst

ChristophWurst commented Oct 19, 2020

Copy link
Copy Markdown
Member

/backport b79e34cac3d8d179027c8 to stable18

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nickvergessennickvergessennickvergessen approved these changes
@skjnldsvskjnldsvskjnldsv approved these changes
@rullzerrullzerAwaiting requested review from rullzer
@gbiggsgbiggsAwaiting requested review from gbiggs
@MorrisJobkeMorrisJobkeAwaiting requested review from MorrisJobke
@georgehrkegeorgehrkeAwaiting requested review from georgehrke
@juliusknorrjuliusknorrAwaiting requested review from juliusknorr

Assignees

No one assigned

Labels

bugfeature: activity and notificationfeature: dav

Projects

None yet

Milestone

Nextcloud 20

Development

Successfully merging this pull request may close these issues.

5 participants

@blizzz@juliusknorr@ChristophWurst@nickvergessen@skjnldsv