share with teams

[hweihwang]

Fixes #142

[blizzz]

I looked at the backend bits only so far.

[hweihwang]

Thanks Arthur @blizzz 🙏 - those are really helpful!

[enjeck]

I was just testing this out, and it looks pretty good!
Just want to confirm one thing. Is it okay that when if I share with a team, and later disable the Teams app, the team display name can not be shown (just the circle ID)? I assume this is expected since we can't access the display name if the app is disabled, but would like to confirm?

[hweihwang]

Thank you @enjeck! That's a really good points, I didn't think about that actually. I think we can hide those shares when circles disabled, that would be better and more appropriate for this case. Let me know if we expect it different like completely remove those shares @juliusknorr @blizzz , thank you!

[juliusknorr]

I think we can hide those shares when circles disabled

Yes, would agree with that. I think we do it the same way in the files app 👍

[enjeck]

I think we can hide those shares when circles disabled

To confirm, in addition to hiding the shares from the sidebar, the tables are not shared with the users in a team?

[hweihwang]

Yes @enjeck, we check from BE so when getting all tables of a user will exclude those circle's shares

[blizzz]

Backendwise looks OK (only nitpicks, not blocking), did not do functional tests so far.

[blizzz]

Suggested change

	private LoggerInterface $logger;
	private bool $circlesEnabled;
	private ?CirclesManager $circlesManager;
	/**
	* @psalm-suppress UndefinedClass
	*/
	public function __construct(
	LoggerInterface $logger,
	IAppManager $appManager
	) {
	$this->logger = $logger;
	$this->circlesEnabled = $appManager->isEnabledForUser('circles');
	$this->circlesManager = null;
	if ($this->circlesEnabled) {
	try {
	$this->circlesManager = Server::get(CirclesManager::class);
	} catch (Throwable $e) {
	$this->logger->warning('Failed to get CirclesManager: ' . $e->getMessage());
	$this->circlesEnabled = false;
	}
	}
	}
	private bool $circlesEnabled;
	private ?CirclesManager $circlesManager;
	/**
	* @psalm-suppress UndefinedClass
	*/
	public function __construct(
	private LoggerInterface $logger,
	IAppManager $appManager
	) {
	$this->circlesEnabled = $appManager->isEnabledForUser('circles');
	$this->circlesManager = null;
	if ($this->circlesEnabled) {
	try {
	$this->circlesManager = Server::get(CirclesManager::class);
	} catch (Throwable $e) {
	$this->logger->warning('Failed to get CirclesManager: ' . $e->getMessage());
	$this->circlesEnabled = false;
	}
	}
	}

nitpick: use property promotion

[blizzz]

What are they about? There should be nothing extra ordinary here?

[blizzz]

same question

[blizzz]

What is psalm complaining about here?

[blizzz]

also wondering here

[blizzz]

Another nitpick: ideal turn 'cirlce' into a constant (and adapt follow up usages).

(Yes, existing group string suffers from the same)