Skip to content

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

Merged
juliusknorr merged 2 commits intonextcloud:masterfrom
mstolf:bugfix/3861/inserted-required-property-in-rename-list-field
Jul 26, 2022
Merged

Inserted required property in the rename list field, to prevent the l…#3862
juliusknorr merged 2 commits intonextcloud:masterfrom
mstolf:bugfix/3861/inserted-required-property-in-rename-list-field

Conversation

@mstolf
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

@juliusknorr juliusknorr 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
Copy Markdown
Member

cc @desperateCoder

@stefan-niedermann
Copy link
Copy Markdown
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?

@juliusknorr
Copy link
Copy Markdown
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