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

Validation fixes #1900

Merged
merged 3 commits into from Mar 14, 2020
Merged

Validation fixes #1900

merged 3 commits into from Mar 14, 2020

Conversation

@TBK
Copy link
Contributor

TBK commented Feb 15, 2020

Fix to #1570

@TBK TBK force-pushed the TBK:validation_fixes branch 2 times, most recently from eaf4b94 to fb8b924 Feb 15, 2020
@ssddanbrown

This comment has been minimized.

Copy link
Member

ssddanbrown commented Mar 3, 2020

Thanks for offering these changes @TBK.

I like the standardising on a single image rules validation message, I can only think it's a mistake that I've left it in two places. We should probable remove the method from the ImageRepo completely to stop future duplication.

In regards to the other changes, Have you figured out exactly what these fix? I'd rather not touch the rules unless I can understand what they fix, even if only to be able to write tests for to prevent happening again.

@TBK TBK force-pushed the TBK:validation_fixes branch from fb8b924 to bcd8a7c Mar 3, 2020
TBK added 3 commits Mar 3, 2020
…mage
@TBK TBK force-pushed the TBK:validation_fixes branch from bcd8a7c to 57f587a Mar 3, 2020
@TBK

This comment has been minimized.

Copy link
Contributor Author

TBK commented Mar 3, 2020

I have changed $request->filled() to $request->hasFile() to check if the http request contains a file.

nullable allows the field to be empty on validation.

With my setup without the changes I can not change the app logo, profile picture or create a shelf/book without an image.

I have also removed getImageValidationRules() from ImageRepo.

@ssddanbrown ssddanbrown added this to the v0.28.3 milestone Mar 14, 2020
@ssddanbrown ssddanbrown merged commit 200772d into BookStackApp:master Mar 14, 2020
3 checks passed
3 checks passed
build (7.2)
Details
build (7.3)
Details
codeclimate All good!
Details
@ssddanbrown

This comment has been minimized.

Copy link
Member

ssddanbrown commented Mar 14, 2020

Thanks again @TBK for these updates.

Have given them a test on my environment and all seems good. I do try to cover any back-end issues with automated tests but, since these issues only occurred in a specific environment, I have been unable to do that here so BookStack on your environment may be a little less stable than others as changes are made but I'm happy to support such updates as this where needed.

@ssddanbrown

This comment has been minimized.

Copy link
Member

ssddanbrown commented Mar 14, 2020

Forgot to say, These have been merged into master to be in the next patch release.

@TBK TBK deleted the TBK:validation_fixes branch Mar 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.