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

Inserted required property in the rename list field, to prevent the l… #3862

Merged
merged 2 commits into from
Jul 26, 2022
Merged

Inserted required property in the rename list field, to prevent the l… #3862

merged 2 commits into from
Jul 26, 2022

Conversation

mstolf
Copy link
Member

@mstolf mstolf commented Jun 10, 2022

When creating a list on the Deck, there is a validation requiring the name of the list to be informed

But when renaming a list, it was possible to keep it without a name, so I put the required field validation

Checklist

  • Code is properly formatted
  • Sign-off message is added to all commits
  • Tests (unit, integration, api and/or acceptance) are included
  • Documentation (manuals or wiki) has been updated or is not required

@stefan-niedermann
Copy link
Member

Usually my opinion is, that this is a validation, and validations belong into the backend (at least). Other clients will otherwise still be able to create / rename lists to blank.
On the other hand this was a breaking change of the REST-API and should be planned well therefore.

@mstolf
Copy link
Member Author

mstolf commented Jun 10, 2022

@stefan-niedermann In this case I followed the list creation pattern, from what I saw it was treated on the front-end too, I understand that in this case the validation would be better on the back-end because it would not open room for errors

Signed-off-by: Julius Härtl <jus@bitgrid.net>
Copy link
Member

@juliushaertl juliushaertl left a comment

Choose a reason for hiding this comment

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

Thanks for the pull request, I pushed a fix to also enforce this on the backend, so LGTM 👍

@stefan-niedermann
Copy link
Member

cc @desperateCoder

@stefan-niedermann
Copy link
Member

@juliushaertl

  1. Will there be a database migration? (e. g. converting Blank to Space?)
  2. How will the API respond to a request that contains Blank? 400? 422? Any proper error message?

@juliushaertl
Copy link
Member

@stefan-niedermann The API will return a 400 Bad Request response as if the title was not set at all with the error message from https://github.com/nextcloud/deck/pull/3862/files#diff-8a98def6b971c75496456ec632f1310cd132642aa188861682457694b47b5096R218

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.

It is possible to leave the name of a list blank by renaming it
3 participants