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

1st try default group notification setting #2858

Merged
merged 12 commits into from Feb 7, 2020

Conversation

jakobroehrl
Copy link
Contributor

@jakobroehrl jakobroehrl commented Jan 28, 2020

Fix #2069
Fix #2085

@nickvergessen
Thanks for your great help 👍

What do you think about the code?
Now the added users have no preselection here:
grafik

@nickvergessen
Copy link
Member

I pushed some commits to clean up the code a bit. Hope this is okay for you.

So now we are missing the dirty parts in spreed/lib/Chat/Notifier.php. Do you want to take care of them as well, or shall I take over. I think it gets a bit/lot more complicated in there.

@nickvergessen nickvergessen added this to the 💚 Next Major (19) milestone Jan 29, 2020
@nickvergessen nickvergessen added 2. developing enhancement feature: chat 💬 Chat and system messages feature: settings ⚙️ Settings and config related issues labels Jan 29, 2020
@jakobroehrl
Copy link
Contributor Author

@nickvergessen
Thank you, tested it.
If I use "All messages" in the admin settings and create a group the chat notifications of the users are:
grafik
This is stil a problem, isn't it?

I look over the Notifier.php yesterday, it was a bit confusing. So if you are willing to explain a little bit more I could do it maybe.

Or there are other small Issues I can help? #774 #1127 #2800 #2226 would be interessting to me, is there an easy one?

@nickvergessen
Copy link
Member

I look over the Notifier.php yesterday, it was a bit confusing. So if you are willing to explain a little bit more I could do it maybe.

Let me try this later.

Or there are other small Issues I can help? #774 #1127 #2800 #2226 would be interessting to me, is there an easy one?

While they sound neat, I think none of them qualifies for "easy". Especially since they should then also work in our mobile apps (apart from the task creation I guess). You could otherwise try to look through the issues of https://github.com/nextcloud/spreed/issues?q=is%3Aissue+is%3Aopen+label%3A%22good+first+issue%22 We try to flag issues with that "good first issue" label where we think it's easy to get started.

Btw, feel free to join our "Talk development" chat at https://cloud.nextcloud.com/call/c7fz9qpr
Might be easier to answer questions more quickly

@nickvergessen
Copy link
Member

If I use "All messages" in the admin settings and create a group the chat notifications of the users are

Fixed now

@jakobroehrl
Copy link
Contributor Author

Fixed now

Sorry, it's not working for me. No matter what I choose in the admin settings, the chat notification for the users is always "@-mention"

@nickvergessen
Copy link
Member

Well most likely because you selected the value once?
Try creating a new conversation and don't touch the setting in the menu

@nickvergessen
Copy link
Member

I fixed the backend. For me the setting is now working as intended and also shows correct in the UI (remember to make build-js the new javascript)

@jakobroehrl
Copy link
Contributor Author

I fixed the backend. For me the setting is now working as intended and also shows correct in the UI (remember to make build-js the new javascript)

It's not working, sorry.
The after a change in the admin config, it looks wired:
grafik

When I relaod the page it's ok again:
grafik

And after creating a new group, it's still the wrong notifiction:
grafik

@marcoambrosini
Copy link
Member

@jakobroehrl it works for me as well.
Are you using 2 separate tabs for talk and settings? I've managed to reproduce the behavior you describe that way.

@jakobroehrl
Copy link
Contributor Author

@jakobroehrl it works for me as well.
Are you using 2 separate tabs for talk and settings? I've managed to reproduce the behavior you describe that way.

No, I do not, this is strange...

@nickvergessen
Copy link
Member

Let's merge this for now and in case we have troubles going forward, we at least know what caused it

@nickvergessen
Copy link
Member

Unit tests are failing.... fixing

@jakobroehrl
Copy link
Contributor Author

Hi, thanks for your work.
How could I support you guys?
The notifications are still open, I don't know if I can do this.

@nickvergessen
Copy link
Member

nickvergessen commented Feb 5, 2020

  • Integration tests seem to 💥

jakobroehrl and others added 2 commits February 7, 2020 11:15
Signed-off-by: Jakob Röhrl <jakob.roehrl@web.de>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen nickvergessen merged commit 4330063 into master Feb 7, 2020
@nickvergessen nickvergessen deleted the enh/group-default-notification branch February 7, 2020 14:47
@nickvergessen
Copy link
Member

/backport to stable18

@backportbot-nextcloud
Copy link

backport to stable18 in #2903

@nickvergessen
Copy link
Member

Thanks again for the start on this @jakobroehrl Feel free to pick any other "good first issue" if you want to continue

@jakobroehrl
Copy link
Contributor Author

@nickvergessen Thanks for the quick release, it's working! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release enhancement feature: chat 💬 Chat and system messages feature: settings ⚙️ Settings and config related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change the default behaviour of notifications in group messages Default setting for notifications
3 participants