Skip to content

[Refactor:Developer] Remove magic methods from Gradeable.php#11702

Merged
bmcutler merged 36 commits into
mainfrom
no-magic
Aug 1, 2025
Merged

[Refactor:Developer] Remove magic methods from Gradeable.php#11702
bmcutler merged 36 commits into
mainfrom
no-magic

Conversation

@lavalleeale

@lavalleeale lavalleeale commented Jun 2, 2025

Copy link
Copy Markdown
Contributor

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

@codecov

codecov Bot commented Jun 2, 2025

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 43.62140% with 137 lines in your changes missing coverage. Please review.
✅ Project coverage is 21.75%. Comparing base (7c33d31) to head (236661f).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@             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              
Flag Coverage Δ
autograder 21.31% <ø> (ø)
js 2.07% <ø> (ø)
migrator 100.00% <ø> (ø)
php 20.76% <43.62%> (+0.18%) ⬆️
python_submitty_utils 80.08% <ø> (ø)
submitty_daemon_jobs 90.72% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

* grade_inquiries?: boolean,
* grade_inquiries_per_component?: boolean,
* discussion_based?: boolean,
* discussion_thread_id?: boolean,

@lavalleeale lavalleeale Jun 2, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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()

@lavalleeale lavalleeale Jun 2, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

Comment thread site/app/models/gradeable/Gradeable.php
Comment thread site/app/models/gradeable/Gradeable.php
@github-project-automation github-project-automation Bot moved this from Seeking Reviewer to Work in Progress in Submitty Development Jun 24, 2025
@github-project-automation github-project-automation Bot moved this from Work in Progress to Awaiting Maintainer Review in Submitty Development Jun 26, 2025
@automateprojectmangement automateprojectmangement Bot moved this from Awaiting Maintainer Review to In Review in Submitty Development Jun 27, 2025
Comment thread site/app/models/gradeable/Gradeable.php
Comment thread site/app/models/gradeable/Gradeable.php Outdated

public function setInstructionsUrl(string $url): void {
$this->instructions_url = $url;
$this->modified = true;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

Comment on lines +2790 to +2800
/**
* 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;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It looks like this setter is only used once, so you could probably just fix that instead of adding all of this complicated logic.

@github-project-automation github-project-automation Bot moved this from In Review to Work in Progress in Submitty Development Jun 27, 2025
@automateprojectmangement automateprojectmangement Bot moved this from Work in Progress to In Review in Submitty Development Jul 29, 2025

@JManion32 JManion32 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code changes look good definitely an improvement for developers. I tinkered around with gradeables and everything functioned as expected.

@bmcutler

Copy link
Copy Markdown
Member

@lavalleeale
Please resolve merge conflicts

@bmcutler bmcutler moved this from In Review to Ready to Merge in Submitty Development Jul 31, 2025
@bmcutler
bmcutler merged commit dfcb4ac into main Aug 1, 2025
26 checks passed
@bmcutler
bmcutler deleted the no-magic branch August 1, 2025 22:18
@github-project-automation github-project-automation Bot moved this from Ready to Merge to Done in Submitty Development Aug 1, 2025
bmcutler pushed a commit that referenced this pull request Aug 7, 2025
### 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? -->
bmcutler pushed a commit that referenced this pull request Aug 8, 2025
### 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? -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

5 participants