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

Populate the mention-notification with the actual message #12009

Merged

Conversation

nickvergessen
Copy link
Member

@nickvergessen nickvergessen commented Oct 24, 2018

Write a comment where you mention a user on a shared file.
The notification so far only says you where mentioned.
Now it includes the text of the comment (as plain text).
With nextcloud/notifications#190 it will also be parsed and the mentions should be clickable (if there where more than your own).

Copy link
Member

@rullzer rullzer left a comment

Choose a reason for hiding this comment

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

🚀

Quick smoke tests looks awesome

Signed-off-by: Joas Schilling <coding@schilljs.com>
@rullzer rullzer force-pushed the feature/noid/populate-notification-message-with-the-comment branch from 1d35217 to d295ff5 Compare October 30, 2018 12:30
Copy link
Member

@blizzz blizzz left a comment

Choose a reason for hiding this comment

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

I did not include the whole message initially to avoid leaking content wherever it may show up. It seems to be the state of the art across apps though, to display it immediately.

In itself it looks good. However, the notifications app itself does not seem to pull notifications (git master), I never see anything… thus, cannot really smoke test. The prepare() method is not being entered either.

Copy link
Member

@blizzz blizzz left a comment

Choose a reason for hiding this comment

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

However, the notifications app itself does not seem to pull notifications (git master), I never see anything… thus, cannot really smoke test. The prepare() method is not being entered either. Another make build-js was missing… works!

@blizzz blizzz merged commit 83cff31 into master Oct 31, 2018
@blizzz blizzz deleted the feature/noid/populate-notification-message-with-the-comment branch October 31, 2018 10:52
@juliusknorr
Copy link
Member

@blizzz Maybe a bit to early, unit tests are failing on master:

https://drone.nextcloud.com/nextcloud/server/12138

@blizzz
Copy link
Member

blizzz commented Oct 31, 2018

damn, scrolled to far and waived it off as unrelated noise :(

@blizzz
Copy link
Member

blizzz commented Oct 31, 2018

Let me have a quick look about adjusting those tests, as they don't seem to be fundamentally broken

@blizzz
Copy link
Member

blizzz commented Oct 31, 2018

take a few minutes more, I add a reverting PR. Sorry for the mess.

@blizzz blizzz restored the feature/noid/populate-notification-message-with-the-comment branch October 31, 2018 15:20
@ChristophWurst ChristophWurst deleted the feature/noid/populate-notification-message-with-the-comment branch October 31, 2018 15:40
@blizzz blizzz added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Oct 31, 2018
@blizzz
Copy link
Member

blizzz commented Oct 31, 2018

@nickvergessen can you reopen with adjusted tests, please?

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.

4 participants