Skip to content

Bugfix/fix orphaned certificate pivot rows#522

Open
Kroffie wants to merge 2 commits into
masterfrom
bugfix/fix_orphaned_certificate_pivot_rows
Open

Bugfix/fix orphaned certificate pivot rows#522
Kroffie wants to merge 2 commits into
masterfrom
bugfix/fix_orphaned_certificate_pivot_rows

Conversation

@Kroffie

@Kroffie Kroffie commented Jun 2, 2026

Copy link
Copy Markdown

Add migration to cleanup old "ghost" pivot entries. Then add logic to detach users from certificates when the certificate gets deleted.

@Kroffie Kroffie self-assigned this Jun 2, 2026
@Kroffie Kroffie requested a review from a team as a code owner June 2, 2026 18:38
@Kroffie Kroffie added the bug Something isn't working label Jun 2, 2026
@codecov

codecov Bot commented Jun 2, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 50.00000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 40.79%. Comparing base (ee24ab9) to head (191e5d3).
⚠️ Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
app/Certificate.php 50.00% 3 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master     #522      +/-   ##
============================================
+ Coverage     40.77%   40.79%   +0.02%     
- Complexity      630      632       +2     
============================================
  Files            99      100       +1     
  Lines          2362     2368       +6     
============================================
+ Hits            963      966       +3     
- Misses         1399     1402       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 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.

Comment thread app/Certificate.php
Comment on lines +17 to +20
static::deleted(function($certificate){
$certificate->users()->detach();
});
}

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.

Note that this defeats most of the purpose of using soft deletes for this model. After all, the certificate itself consists merely of a name and abbreviation. The most valuable certificate data lies in its relations, i.e., which users the certificate is/was assigned to.

In Laravel, soft deletes behave the same as actual deletes, unless the deleted records are explicitly requested using ->withTrashed(). Note that this allows us to keep the "pivot rows" of removed certificates while automatically hiding them when not requested. This makes restoring accidental deletes possible.


Observe that the certificate BelongsTo relation on the User model explicitly requests the certificates withTrashed:

return $this->belongsToMany(Certificate::class)
->withTimestamps()->withTrashed();

In case we want to exclude removed certificates from the user's certificate overview (which IMO we do), we should simply remove the ->withTrashed() from that line. That would remove the need to detach on delete as well as the migration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants