Fix setting submission to null when submission is disabled #1311
Conversation
EvilMonkey09
requested review from
Jonas-Sander and
nilsreichardt
as code owners
WalkthroughThe update in the Changes
Assessment against linked issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (1)
- app/lib/homework/homework_dialog/homework_dialog_bloc.dart (1 hunks)
Additional comments: 1
app/lib/homework/homework_dialog/homework_dialog_bloc.dart (1)
- 753-757: The logic for resetting the
submissionTimetonullwhen the "Mit Abgabe" feature is deactivated is correctly implemented. This change directly addresses the issue described in the PR objectives by ensuring thatsubmissionTimeis only set when submissions are enabled and explicitly cleared otherwise. This is a crucial fix for the bug where deactivating the submission feature did not reset thesubmissionTime, leading to incorrect display information on the dashboard.However, it's important to ensure that this change does not inadvertently affect other parts of the application that rely on the
submissionTimeor_dateSelectionproperties. Given the scope of the change, it seems well-contained, but a thorough testing strategy, including unit tests and integration tests, should be employed to verify the behavior in various scenarios, such as toggling the submission feature on and off, editing existing homework assignments, and creating new ones.
| submissionTime: event.newSubmissionsOptions.submissionTime ?? | ||
| Time(hour: 23, minute: 59)); | ||
| } else { | ||
| _dateSelection = _dateSelection.copyWith(submissionTime: null); | ||
| _dateSelection = _DateSelection( | ||
| dueDate: _dateSelection.dueDate, | ||
| dueDateSelection: _dateSelection.dueDateSelection, | ||
| submissionTime: null, | ||
| ); | ||
| } | ||
|
|
||
| emit(_getNewState()); | ||
| }, | ||
| ); |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-752]
Overall, the HomeworkDialogBloc class demonstrates a comprehensive approach to managing the state and events within the homework dialog of the Sharezone app. The use of the Bloc pattern for state management is appropriate for the complexity of the dialog's functionality, allowing for clear separation between UI events and state changes. The class handles a variety of events related to homework creation and editing, including title changes, due date adjustments, course selection, and more, which are all critical for providing a flexible and user-friendly interface for managing homework assignments.
A few areas for potential improvement and considerations include:
- Error Handling: Ensure that all external calls, especially asynchronous ones, have appropriate error handling to prevent the application from entering an inconsistent state or failing silently. This is particularly important for operations that modify the state of homework assignments or interact with external services.
- Testing: Given the complexity of the
HomeworkDialogBlocand its central role in the homework management feature, a thorough suite of unit and integration tests is essential. Tests should cover the various states the Bloc can enter, the handling of different events, and the interaction with external services through theHomeworkDialogApi. - Documentation: While the code is generally well-structured and readable, adding more detailed comments and documentation, especially for public methods and complex logic, would enhance maintainability and ease future development efforts.
While the changes in this PR are focused and address the specific issue at hand, these additional considerations can further improve the robustness and maintainability of the homework dialog feature.
|
Hi, thanks! :) I guess the test should check that if one enables and disables the submissions for a homework and then saves the homework, that the |
nilsreichardt
changed the title
|
@Jonas-Sander I added a small test. Could you also review? |
nilsreichardt
added
the
safe to test
github-actions
bot
removed
the
safe to test
|
Visit the preview URL for this PR (updated for commit 4e8deb7): https://sharezone-test--pr1311-fix-duetime-nullfals-cg3wfqr1.web.app (expires Sun, 03 Mar 2024 10:15:23 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 4cb3ae61e1e018abfd9841fd3239f5b49ccc034b |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
nilsreichardt
added
the
safe to test
github-actions
bot
removed
safe to test
When the "Abgabe" was activated, the submissionTime was set to 23:59. If you deactivated the "Abgabe" the submissionTime wasn't set to null, because of the _dateSelection.copyWith which ignores null.
This caused the problem, that in the Dashboard the dueDate was displayed in red and not "Bis morgen!".
Issue #1306
Closes #1306
Summary by CodeRabbit