Skip to content

Publish notification when adding user#28

Merged
PVince81 merged 1 commit into
masterfrom
notification
Mar 27, 2017
Merged

Publish notification when adding user#28
PVince81 merged 1 commit into
masterfrom
notification

Conversation

@PVince81

@PVince81 PVince81 commented Feb 24, 2017

Copy link
Copy Markdown
Contributor

Fixes #4

@PVince81 PVince81 added this to the 10.0 milestone Feb 24, 2017
@PVince81

Copy link
Copy Markdown
Contributor Author
  • discard/kill that notification if the user membership was removed before the notification is shown or the group was deleted

@PVince81

PVince81 commented Mar 20, 2017

Copy link
Copy Markdown
Contributor Author
  • BUG: router doesn't return correct link

@PVince81

Copy link
Copy Markdown
Contributor Author

Ready for review @jvillafanez @DeepDiver1975 @IljaN @VicDeo

I'll investigate how to kill the notification in case of deletion separately, seems to be tricky.

Comment thread appinfo/app.php Outdated

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.

cries for info.xml enhancement ....

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

if we had a generic service registration we wouldn't need to enhance core for everything out there

<services><service type="\OC\INotifier">\OCA\CustomGroups\Notifier</service></services>

then core (or any app) could just find all services of a given type/namespace.

That was what I hoped to achieve with https://central.owncloud.org/t/service-based-app-imlpementation-to-solve-inter-app-dependencies/1615

Comment thread tests/unit/Dav/GroupsCollectionTest.php Outdated

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.

use ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I could... but I somehow hate having IManager without knowing what a "manager" is.
This should have been called INotificationManager in the first place.

Or I could use use xxx as yyy ?

Comment thread tests/unit/NotifierTest.php Outdated

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.

parent:setup?

Comment thread tests/unit/NotifierTest.php Outdated

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.

mock?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I don't want to mock it... too many calls to it and in the end the default one works fine as it will return the same texts untranslated but formatted

@PVince81

Copy link
Copy Markdown
Contributor Author

@DeepDiver1975 fixed the namespace use and added parent::setUp.

Other pieces aren't blockers from my POV.

@PVince81 PVince81 merged commit 7290263 into master Mar 27, 2017
@PVince81 PVince81 deleted the notification branch March 27, 2017 13:42
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.

Send notification when added to group

2 participants