Conversation
…and setter methods, and update phpstan baseline for type consistency
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #11702 +/- ##
============================================
+ Coverage 21.60% 21.75% +0.15%
- Complexity 9366 9459 +93
============================================
Files 267 267
Lines 35927 36146 +219
Branches 474 474
============================================
+ Hits 7761 7863 +102
- Misses 27696 27813 +117
Partials 470 470
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
| * grade_inquiries?: boolean, | ||
| * grade_inquiries_per_component?: boolean, | ||
| * discussion_based?: boolean, | ||
| * discussion_thread_id?: boolean, |
There was a problem hiding this comment.
discussion_thread_ids are strings, someone probably got confused with the type of the line above
| $graded_gradeable = $this->core->getQueries()->getGradedGradeable( | ||
| $gradeable, | ||
| $user_id, | ||
| $gradeable->isTeamAssignment() |
There was a problem hiding this comment.
The third argument of the query is the team name, so this boolean will never have an effect. It likely should be updated to have a correct value, but this change will not cause a regression
…ontroller and SubmissionController
…d related components
…across multiple components
|
|
||
| public function setInstructionsUrl(string $url): void { | ||
| $this->instructions_url = $url; | ||
| $this->modified = true; |
There was a problem hiding this comment.
Is setting $modified part of why magic methods are useful for this class? I don't like the idea of having to set this variable in every setter, present and future.
There was a problem hiding this comment.
I agree, and I think that we should not be tracking $modified as it is infrequently accessed and I don't believe there is a code path that checks it when it could be false.
| /** | ||
| * Sets the discussion thread id for this gradeable | ||
| * @param array<string>|array<int>|int|string $discussion_thread_id | ||
| */ | ||
| public function setDiscussionThreadId($discussion_thread_id): void { | ||
| # If $discussion_thread_id is json encoded, decode it | ||
| if (is_string($discussion_thread_id) && is_array(json_decode($discussion_thread_id, true))) { | ||
| $discussion_thread_id = json_decode($discussion_thread_id, true); | ||
| } | ||
| $this->discussion_thread_id = array_map('intval', (array) $discussion_thread_id); | ||
| $this->modified = true; |
There was a problem hiding this comment.
It looks like this setter is only used once, so you could probably just fix that instead of adding all of this complicated logic.
…r better handling
JManion32
left a comment
There was a problem hiding this comment.
Code changes look good definitely an improvement for developers. I tinkered around with gradeables and everything functioned as expected.
|
@lavalleeale |
### Why is this Change Important & Necessary? <!-- Include any GitHub issue that is fixed/closed using "Fixes #<number>" or "Closes #<number>" syntax. Alternately write "Partially addresses #<number>" or "Related to #<number>" as appropriate. --> #11702 Magic Methods PR stated that team_lock_date is ?DateTime, which is true, but getting the value actually returns a DateTime, casuing a frog robot crash ``` Oh no! Something irrecoverable has happened... app\exceptions\OutputException (Code: 0) thrown in /usr/local/submitty/site/app/libraries/Output.php (Line 412) by: throw new OutputException("{$e->getMessage()} in {$e->getFile()}:{$e->getLine()}"); Message: An exception has been thrown during the rendering of a template ("app\models\gradeable\Gradeable::getTeamLockDate(): Return value must be of type DateTime, null returned") in "admin/admin_gradeable/AdminGradeableDates.twig" at line 14. in /usr/local/submitty/site/app/templates/admin/admin_gradeable/AdminGradeableDates.twig:14 Stack Trace: #0 /usr/local/submitty/site/app/libraries/Output.php(425): app\libraries\Output->renderTwigTemplate() #1 /usr/local/submitty/site/app/controllers/admin/AdminGradeableController.php(617): app\libraries\Output->renderTwigOutput() #2 /usr/local/submitty/site/app/controllers/admin/AdminGradeableController.php(32): app\controllers\admin\AdminGradeableController->editPage() #3 unknown file(unknown line): app\controllers\admin\AdminGradeableController->editGradeableRequest() #4 /usr/local/submitty/site/app/libraries/routers/WebRouter.php(254): call_user_func_array() #5 /usr/local/submitty/site/app/libraries/routers/WebRouter.php(232): app\libraries\routers\WebRouter->run() #6 /usr/local/submitty/site/public/index.php(147): app\libraries\routers\WebRouter::getWebResponse() USER: instructor ``` ### What is the New Behavior? <!-- Include before & after screenshots/videos if the user interface has changed. --> Like all other dates, team lock date now returns the correct value, `?\DateTime` ### What steps should a reviewer take to reproduce or test the bug or new feature? ### Automated Testing & Documentation <!-- Is this feature sufficiently tested by unit tests and end-to-end tests? If this PR does not add/update the necessary automated tests, write a new GitHub issue and link it below. Is this feature sufficiently documented on submitty.org? Link related PRs or new GitHub issue to update documentation. --> We should check creating and modifying dates for a checkpoint gradeables #11982 11982 ### Other information <!-- Is this a breaking change? Does this PR include migrations to update existing installations? Are there security concerns with this PR? -->
### Why is this Change Important & Necessary? <!-- Include any GitHub issue that is fixed/closed using "Fixes #<number>" or "Closes #<number>" syntax. Alternately write "Partially addresses #<number>" or "Related to #<number>" as appropriate. --> From #11702, On production, we have gradeables that have already been created. When the gradeable gets created, we `json_encode('{}')` as the default value. This means that when it gets inserted into the database, instead of `{}` being inserted, we actually insert `"{}"` instead, which is a string. This means that the new code, which decodes it once, will not work as it will be decoded to `"{}"` instead of `[]`, which throws of frog robot. We can either: 1. Decoding it twice would allow for us to get `[]` instead of string `"{}"`. 2. Make a database migration for this situation where we specifically patch this one instance ### What is the New Behavior? <!-- Include before & after screenshots/videos if the user interface has changed. --> Make a database migration for this situation where we specifically patch this one instance. I feel like this is better and the other way is a little hacky ### What steps should a reviewer take to reproduce or test the bug or new feature? Make a gradeable before #11702. Move to after #11702, edit gradeable, see error ### Automated Testing & Documentation <!-- Is this feature sufficiently tested by unit tests and end-to-end tests? If this PR does not add/update the necessary automated tests, write a new GitHub issue and link it below. Is this feature sufficiently documented on submitty.org? Link related PRs or new GitHub issue to update documentation. --> This could not have been prevented that easily with e2e test. The initial state would have to be from the previous course creations, and then switching to the new version. We could do this to prevent this error, but it would be very hard. ### Other information <!-- Is this a breaking change? Does this PR include migrations to update existing installations? Are there security concerns with this PR? -->
Why is this Change Important & Necessary?
Gradeable.php previously relied on magic methods.
This caused challenges for upgrading PHPUnit and and continuing to support PHPStan for static type analysis.
PR #11726 mostly addressed these concerns.
However magic methods have caused confusion for junior Submitty developers.
AND the code still contained typos/errors in the types of these methods.
What is the New Behavior?
This PR removes the magic methods usage in one file.
The getters and setters are manually created.
What steps should a reviewer take to reproduce or test the bug or new feature?
Automated Testing & Documentation
Other information