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

Can't have forward slash (/) under Settings → Notifications → Mention and keywords #18707

Closed
SethFalco opened this issue Aug 24, 2021 · 9 comments
Labels
A-Notifications A-User-Settings O-Uncommon Most users are unlikely to come across this or unexpected workflow S-Minor Impairs non-critical functionality or suitable workarounds exist T-Defect

Comments

@SethFalco
Copy link

SethFalco commented Aug 24, 2021

Steps to reproduce

  1. Navigate to Settings → Notifications → Mention and keywords
  2. In the Keyword field, input any text containing a forward slash. (/)
  3. Select Add

What happened?

A modal appears with the following text.

Error saving notification preferences

An error occurred whilst saving your notification preferences.

And the content of the Notifications setting changes to.

Notifications

There was an error loading your notification settings.

Peek 2021-08-24 10-27


I can just flip to another page, then return to Notifications settings to access the page again. (The keyword does not get added.)

What did you expect?

I would expect the keyword to get added as it would for any other keyword.

If for whatever reason, not that I can think of one, a / cannot be used in a keyword for a logical/technical reason, then I would expect the field to have validation explicitly warning the user. (Or handle it gracefully one way or another.)

Operating system

Ubuntu 20.04

Application version

Element version: 1.8.1 Olm version: 3.2.3

How did you install the app?

apt: element-desktop/unknown,now 1.8.1 amd64 [installed]

Have you submitted a rageshake?

No

@Palid
Copy link
Contributor

Palid commented Aug 25, 2021

It seems that / is rejected by backend, we need to add some front-end validation for that too.

@Palid Palid added O-Uncommon Most users are unlikely to come across this or unexpected workflow S-Minor Impairs non-critical functionality or suitable workarounds exist labels Aug 25, 2021
@Palid
Copy link
Contributor

Palid commented Aug 25, 2021

It also shouldn't just show There was an error loading your notification settings after showing modal that something has failed.
CleanShot 2021-08-25 at 18 18 58@2x

@Palid Palid closed this as completed Aug 25, 2021
@SethFalco
Copy link
Author

Sorry @Palid, just need to verify.
Did you close this issue in intentionally in favor of 18778 or did you perhaps target the wrong issue?

It doesn't appear to be related?

@SimonBrandner
Copy link
Contributor

Yeah, seems unrelated, @Palid, I think you closed the wrong issue?

@SimonBrandner SimonBrandner reopened this Aug 25, 2021
@Palid
Copy link
Contributor

Palid commented Aug 25, 2021

@SimonBrandner yeah, seems like I got entangled within open tabs. I'll fix it tomorrow, thanks for catching!

@Palid
Copy link
Contributor

Palid commented Aug 26, 2021

Okay, fixed incorrectly referenced issues, thanks for figuring this out @SimonBrandner !

@SethFalco
Copy link
Author

I'm not completely familiar with the workflow of the project, but could it be the project needs to be changed back manually too?

image

@Palid
Copy link
Contributor

Palid commented Aug 26, 2021

I'm not completely familiar with the workflow of the project, but could it be the project needs to be changed back manually too?

image

Yes, you're right. Fixed that too.

@SimonBrandner
Copy link
Contributor

Closing in favour of #18839

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Notifications A-User-Settings O-Uncommon Most users are unlikely to come across this or unexpected workflow S-Minor Impairs non-critical functionality or suitable workarounds exist T-Defect
Projects
None yet
Development

No branches or pull requests

3 participants