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

feat: Improve error messages for invalid submissions #2533

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Koc
Copy link
Collaborator

@Koc Koc commented Jan 29, 2025

Right now it is really hard to understand for end users why form can't be submitted. This PR aims to improve validation messages.

@Koc Koc force-pushed the feature/improve-error-messages branch 2 times, most recently from 23b8286 to a82cc79 Compare January 29, 2025 15:22
Copy link

codecov bot commented Jan 29, 2025

Codecov Report

Attention: Patch coverage is 81.81818% with 4 lines in your changes missing coverage. Please review.

Project coverage is 43.43%. Comparing base (19feceb) to head (ccc9c19).
Report is 8 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #2533   +/-   ##
=========================================
  Coverage     43.43%   43.43%           
- Complexity      882      885    +3     
=========================================
  Files            77       77           
  Lines          3361     3361           
=========================================
  Hits           1460     1460           
  Misses         1901     1901           

@Koc Koc requested review from Chartman123 and susnux January 29, 2025 15:27
@Chartman123 Chartman123 added enhancement New feature or request php PHP related ticket 3. to review Waiting for reviews labels Jan 29, 2025
@Chartman123 Chartman123 changed the title Improve error messages for invalid submissions feat: Improve error messages for invalid submissions Jan 29, 2025
@Chartman123 Chartman123 added this to the 5.0 milestone Jan 29, 2025
Copy link
Collaborator

@Chartman123 Chartman123 left a comment

Choose a reason for hiding this comment

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

Nice one :)

Copy link
Collaborator

@susnux susnux left a comment

Choose a reason for hiding this comment

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

I dont think this makes sense, what is the use case?
It is nothing shown to end users, that should already be done in the frontend and if it is not then there is a bug in the frontend that should be fixed.

So this is for developers that use the API?
In this case we should refactor the function at all (validateSubmission) to be void and throw instead on error.
Either InvalidArgumentException if we keep this developers only I would prefer this.
Otherwise an \OCP\HintException which would allow translated error messages for frontend - but here again we should never allow to submit something invalid so a frontend validation is wrong.

@Koc Koc force-pushed the feature/improve-error-messages branch 3 times, most recently from 77190f7 to fe10d06 Compare February 10, 2025 06:04
@Koc
Copy link
Collaborator Author

Koc commented Feb 10, 2025

@susnux yeah, it's mostly for API usage. I've refactored method to throw an InvalidArgumentException as you suggested.

Psalm failures looks unrelated

This comment was marked as off-topic.

@Koc Koc force-pushed the feature/improve-error-messages branch from fe10d06 to ccc9c19 Compare February 19, 2025 23:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews enhancement New feature or request feedback-requested php PHP related ticket
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants