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

Suggest: Handle error messages from the server #1047

Closed
mal-tee opened this issue Mar 19, 2023 · 5 comments
Closed

Suggest: Handle error messages from the server #1047

mal-tee opened this issue Mar 19, 2023 · 5 comments
Assignees
Labels

Comments

@mal-tee
Copy link
Member

mal-tee commented Mar 19, 2023

I want the backend to be able to reject suggestions with a message. Before implementing this in the backend, I will implement support for it in the frontend.

This will allow us to filter stuff in the backend (e.g. white space changes) to reduce noise in the data repo, without confusing users.

@mal-tee mal-tee added the UI/UX label Mar 19, 2023
@mal-tee mal-tee self-assigned this Mar 19, 2023
@baltpeter
Copy link
Member

baltpeter commented Mar 20, 2023

We do have this already but that's not what you're looking for?

website/src/suggest-edit.js

Lines 312 to 319 in e98e8e5

case 400:
if (json.path) {
document
.querySelector(`tr[data-schema_id='${json.path.join('.').replace(/^data/, '$')}']`)
?.classList.add('invalid');
}
flash(<FlashMessage type="error">{t('invalid-request', 'suggest')}</FlashMessage>);
break;

@mal-tee
Copy link
Member Author

mal-tee commented Mar 20, 2023

Hmm, right. It's a bit generic and w/o a custom message, but should do the trick.

@baltpeter
Copy link
Member

baltpeter commented Mar 20, 2023

I mean, you could also add a way to set a custom error message for those (but you would have to handle translations). I'm just not quite sure what you want to implement yet. :D

@mal-tee
Copy link
Member Author

mal-tee commented Mar 21, 2023

I'm just not quite sure what you want to implement yet. :D

E.g. I want the backend to check if value updates are just white space changes and omit them to reduce noise like this:

https://github.com/datenanfragen/data/pull/2134/files#diff-2e2c693eeb489acb22a617dd6fe60a96dd1f1db5b2c04418157418d52b5e49d5L16-R18

But this could result in edge cases where the server decides that a suggestion isn't post-worthy and therefore won't generate a PR. Since this is different than a straight up error, I thought we might want to cover this via a message.

But since you brought up the issue of translating these messages, I think we could just use the generic error. :D

I also have other ideas, e.g. withholding PRs that contain the string "gmail"... :D

@mal-tee mal-tee closed this as completed Mar 21, 2023
@baltpeter
Copy link
Member

E.g. I want the backend to check if value updates are just white space changes and omit them to reduce noise like this:

Oh btw, I think you've suggested this before and I disagreed but I have now changed my mind on this. Let's force-format everything: datenanfragen/data#2165, datenanfragen/data#2166

But since you brought up the issue of translating these messages, I think we could just use the generic error. :D

Wouldn't be too bad. You would just return a translation key from the server. shrug

I also have other ideas, e.g. withholding PRs that contain the string "gmail"... :D

Oh, that's a good idea. But unfortunately:

https://github.com/datenanfragen/data/blob/4965796bb876a20788e131f1b3a7d896166f83ad/companies/tierschutzallianz.json#L12

You'd need a way to have the user confirm that this really is the company's email and not theirs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

2 participants