fix: close the race in create_certificate that lets duplicate certificates through

[suparikoli]

Fixes #2408.

create_certificate() calls is_certified(course) and only builds a new certificate if that comes back empty - there's nothing between the check and the save() that stops two requests from both reading "not certified yet." A doubled click on "Get Certificate" is realistically enough to hit that window, and the result is two LMS Certificate rows for the same member and course.

There was already a PR for this (#2409), closed a few months back after going stale. @raizasafeel's review on it laid out the right shape for the fix - a DB-level unique constraint is the only thing that actually closes the window, since an if exists check has the same race whether it's in Python or in validate(); keep the fast path for the normal case, but catch the constraint violation on the way out and hand back the certificate that won instead of erroring. This picks that up.

Added frappe.db.add_unique("LMS Certificate", ["member", "course", "batch_name"]), applied through a patch (existing databases can already have duplicates sitting around, so it dedupes first, keeping the earliest of each set) and also directly in after_install for fresh sites, which don't run patches. That part mirrors delete_duplicate_course_progress almost exactly, which hit the same shape of bug for LMS Course Progress a few months ago.

One thing I changed from the (member, course) framing in that review comment: a certificate can also be earned through a batch instead of a course, and course is blank on those rows. A two-column constraint on just (member, course) would treat every batch certificate a member holds as a duplicate of the next one, since they'd all share the same blank course. Made it (member, course, batch_name) instead, which keeps course-certs and batch-certs distinguishable from each other while still catching a real duplicate of either kind.

create_certificate() now wraps the save() in a try/except for frappe.UniqueValidationError and, on that path, looks up whichever certificate actually won and returns it - same shape as what a normal is_certified() hit returns, so nothing downstream needs to care whether it was the fast path or the recovered-from-a-lost-race path.

The patch does delete rows (the duplicate certificates, keeping the earliest per member+course/batch), so it's worth a closer look than the rest of the diff - flagging that up front rather than leaving it for someone to notice.

For tests, added two to what was an empty test_lms_certificate.py. The first just calls create_certificate twice in a row and checks it's idempotent - not new behavior, but the existing fast path had no coverage either. The second is the one that actually exercises the fix: it mocks is_certified to return None on the second call even though the first call's certificate already exists, which is the only way I could find to force the actual race window deterministically in a single-threaded test rather than just re-proving the fast path works.

I wasn't able to run this against a live site from where I'm working - no bench set up in this environment - so I traced it carefully against current develop instead: pulled the real create_certificate/is_certified source, followed UniqueValidationError and add_unique into frappe/model/base_document.py and frappe/database/mariadb/database.py to confirm exactly when Frappe raises that exception and what add_unique does at the SQL level, and checked the enable_certification default on LMS Course (it's 0) so the test explicitly turns it on rather than assuming the fixture already has it. Ran ruff check and ruff format --check against the changed files, both clean. If there's a lighter way to get this running against an actual site that I'm missing, happy to go do that too.

[greptile-apps]

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

_{Reviews (2): Last reviewed commit: "fix: lock the course row, and fix a NULL..." | Re-trigger Greptile}

[greptile-apps]

Patch recreates existing index

The patch calls add_unique unconditionally, so rerunning it after index creation but before patch completion is recorded can fail migration recovery because the index already exists.

Context Used: Guidelines for reviewing Frappe Framework applicat... (source)

Knowledge Base Used: LMS App Lifecycle: Hooks, Auth, Permissions, and Install

[suparikoli]

Pushed an update after Greptile's review and after rebasing onto develop to pick up the security patches merged there in the meantime. Two things changed, both real:

Greptile was right that course/batch_name save as SQL NULL, not "", on a certificate that never touches whichever of the two it doesn't use - I'd assumed the usual Frappe "unset = empty string" convention applied here without actually checking, and it doesn't for a plain frappe.get_doc() that never sets the field. Since NULL != NULL in a unique index, the constraint as originally written would have let two course-only certificates for the same member+course both sit at batch_name=NULL without colliding - quietly doing nothing for the exact case this PR is about. validate() now normalizes both fields to "" before every save, and the patch normalizes existing NULLs the same way before it groups rows to dedupe them.

Separately, lms/lms/enrollment_constraints.py landed on develop while this was open - the same fix for LMS Enrollment/LMS Batch Enrollment, going by a FOR UPDATE lock on the parent row before the check, with the unique constraint kept as a backstop for whatever bypasses the controller. That's a more direct fix than catching UniqueValidationError after an insert that didn't need to be attempted, so create_certificate() now takes the same kind of lock on the course row that enroll_in_course() already takes, before its own is_certified() check. Kept the exception handling as the backstop, same as that module does for its own indexes.

Also corrected the race-recovery test: it patched is_certified() but not validate_duplicate_certificate(), so the second call was actually being caught by the existing, already-working duplicate check before it ever reached the code this PR added - passing, but not testing what it said it was testing. Added a lock-ordering test and a "the friendly error still fires for an ordinary sequential duplicate" test too, both adapted from the equivalent ones in the new tests/test_enrollment_races.py.