Skip to content

[Bugfix:Forum] Handle deleted reply/thread redirects#12970

Merged
bmcutler merged 5 commits into
Submitty:mainfrom
jndlansh:bugfix/forum-notifications
Jul 6, 2026
Merged

[Bugfix:Forum] Handle deleted reply/thread redirects#12970
bmcutler merged 5 commits into
Submitty:mainfrom
jndlansh:bugfix/forum-notifications

Conversation

@jndlansh

@jndlansh jndlansh commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Why is this Change Important & Necessary?

Closes #12814
Clicking a notification for deleted forum content (thread or reply) opens a blank page instead of redirecting the user somewhere safe. This happened because when a post/thread was deleted, the notification metadata was set to json_encode([]) no URL at all, causing to render a blank page.

What is the New Behavior?

  • Deleted thread notification → redirects to the forum index page
  • Deleted post/reply notification → redirects to the parent thread page

What steps should a reviewer take to reproduce or test the bug or new feature?

  1. Create a forum thread as one user
  2. As another user with grading permissions, delete the thread
  3. Open the resulting notification → should redirect to the forum index
  4. Repeat with deleting only a reply in a thread that still exists → should redirect to the parent thread page
  5. Try all possible combinations.

Automated Testing & Documentation

No new test or documentation

Other information

No breaking changes, no migrations needed.

@github-project-automation github-project-automation Bot moved this to Seeking Reviewer in Submitty Development Jun 30, 2026
@jndlansh jndlansh changed the title [Bugfix:Forum] Handle deleted reply/thread redirects from Notifications [Bugfix:Forum] Handle deleted reply/thread redirects Jun 30, 2026
@codecov

codecov Bot commented Jun 30, 2026 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 15 lines in your changes missing coverage. Please review.
✅ Project coverage is 21.40%. Comparing base (c33d434) to head (c34bf85).

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##               main   #12970      +/-   ##
============================================
- Coverage     21.41%   21.40%   -0.01%     
- Complexity    10002    10007       +5     
============================================
  Files           277      277              
  Lines         37418    37430      +12     
  Branches        512      512              
============================================
  Hits           8013     8013              
- Misses        28904    28916      +12     
  Partials        501      501
Flag Coverage Δ
autograder 21.16% <ø> (ø)
js 1.98% <ø> (ø)
migrator 100.00% <ø> (ø)
php 20.36% <0.00%> (-0.01%) ⬇️
python_submitty_utils 80.08% <ø> (ø)
submitty_daemon_jobs 91.13% <ø> (ø)
vue 100.00% <ø> (ø)

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.

@jndlansh
jndlansh marked this pull request as draft June 30, 2026 18:23
@automateprojectmangement automateprojectmangement Bot moved this from Seeking Reviewer to Work in Progress in Submitty Development Jun 30, 2026
@jndlansh
jndlansh marked this pull request as ready for review July 1, 2026 13:56
@automateprojectmangement automateprojectmangement Bot moved this from Work in Progress to Seeking Reviewer in Submitty Development Jul 1, 2026
dagemcn
dagemcn requested changes Jul 1, 2026
View reviewed changes

@dagemcn dagemcn left a comment

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.

Looked at the code and functionality of this PR fairly extensively. One tiny nitpick, if you create a thread and reply to that thread, then delete the reply first and then the original post, you will get two notifications. Clicking on the reply notification will bring you to the forum index page with a banner that says "The requested thread does not exist".

Image

I think we should either show this banner in all occurrences where the requested reply/thread etc. does not exist or we should not show it in any circumstance. I understand this is a very small nitpick, and if you'd rather wait until the Vue migration of the discussion forum to handle this that's fine with me as well considering this banner only shows up in one tiny circumstance and is only a tiny discrepancy. Also, please fix failing php lint, seems like only one small change required. After that, I am happy to approve.

@github-project-automation github-project-automation Bot moved this from Seeking Reviewer to Work in Progress in Submitty Development Jul 1, 2026
@jndlansh

jndlansh commented Jul 1, 2026

Copy link
Copy Markdown
Contributor Author

I have tried to make a unified Toast for all such scenarios that says : The content for this notification has been deleted or is no longer available.
I guess that should do it.

dagemcn
dagemcn approved these changes Jul 1, 2026
View reviewed changes

@dagemcn dagemcn left a comment •
edited
Loading

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.

Code looks good and I am a fan of the banner! I will approve. Also, PHP seems to still be having issues.

@github-project-automation github-project-automation Bot moved this from Work in Progress to Awaiting Maintainer Review in Submitty Development Jul 1, 2026
@bmcutler
bmcutler merged commit 412f878 into Submitty:main Jul 6, 2026
51 of 53 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dagemcn dagemcn dagemcn approved these changes

Assignees

No one assigned

Labels

None yet

Projects

Submitty Development
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Bug:Notifications] Clicking deleted forum notifications opens a blank page

3 participants

@jndlansh @dagemcn @bmcutler