-
-
Notifications
You must be signed in to change notification settings - Fork 335
Feature/add server validation comments #959
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
base: master
Are you sure you want to change the base?
Feature/add server validation comments #959
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was really confused about this till I saw that it's one of flutter's generated files 😅
| import 'package:wger/providers/add_exercise.dart'; | ||
|
|
||
| @GenerateMocks([AddExerciseProvider]) | ||
| import 'add_exercise_translation_test.mocks.dart'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should probably be add_exercise_screen_test.mocks.dart
| mockAddExerciseProvider = MockAddExerciseProvider(); | ||
| }); | ||
|
|
||
| group('Language validation - correct language', () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note that since we're mocking the provider, these don't actually test anything. The backend already has some tests for that so the most we could do here is make sure validateLanguage makes the correct requests to the server
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added 14 unit tests verifying correct validateLanguage calls
| GlobalKey<FormState>(), | ||
| ]; | ||
|
|
||
| Future<bool> _validateLanguageOnServer(BuildContext context) async { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had hoped we could add something to the fields themselves so that the logic would be directly in e.g. Step3Description or AddExerciseTextArea. I find it very baffling that flutter does not have support for async validators, then this would be way easier
lib/screens/add_exercise_screen.dart
Outdated
| bool _isLoading = false; | ||
| bool _isValidating = false; | ||
| Widget errorWidget = const SizedBox.shrink(); | ||
| String? _validationError; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can take a look at FormHttpErrorsWidget, you can give it a WgerHttpException directly and it will render it nicely
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now using FormHttpErrorsWidget instead of plain Text widget
|
@Richard141201 I hadn't time to review this, sorry. It's approved so it should count towards hacktoberfest, even if it's not merged. Will merge it as soon as I can |
feat: Add step-level language validation for exercise descriptions
Validate description language immediately when clicking "Next" instead
of at final form submit. Shows loading indicator and inline errors.
Changes:
Tests: