Updated view_user_cert to support all certificates. #681

Raza403 · 2025-05-01T08:39:21Z

Previously, this was working only for some specific certificates.

mdjnelson · 2025-05-01T18:47:26Z

Can you please look at failing CI pipeline please?

mdjnelson · 2025-06-23T19:39:35Z

Please rebase on the latest MOODLE_404_STABLE branch as I have bumped the Moodle Plugin CI version.

Raza403 · 2025-06-24T10:17:37Z

@mdjnelson rebased.

mdjnelson · 2025-07-06T11:31:27Z

This is still failing. Please address the errors you can see at https://github.com/mdjnelson/moodle-mod_customcert/pull/681/checks.

mdjnelson · 2025-07-06T11:32:51Z

classes/certificate.php

-     *
-     * @return string
-     */
-    public static function generate_code(): string {


Why is the return type being removed?

mdjnelson · 2025-07-06T11:33:03Z

classes/certificate.php

-    private static function generate_code_upper_lower_digits(): string {
-        return random_string(10);
+    // Upper/lower/digits random string
+    private static function generate_code_upper_lower_digits() {


Please specify a return type.

mdjnelson · 2025-07-06T11:33:16Z

classes/certificate.php

-            random_int(0, 9999)
-        );
+    // Digits with hyphens
+    private static function generate_code_digits_with_hyphens() {


Again, return type missing.

mdjnelson · 2025-07-06T11:33:42Z

db/upgrade.php

    }

+    // Drop the unique index on 'code' and add a non-unique one.
    if ($oldversion < 2024042210) {


Why are we touching this upgrade step?

mdjnelson · 2025-07-06T11:34:22Z

element/qrcode/classes/element.php

            }

-            $qrcodeurl = new \moodle_url('/mod/customcert/verify_certificate.php', $urlparams);
+            $qrcodeurl = new \moodle_url('https://app.pacificmedicaltraining.com/verify', $urlparams);


You have hardcoded a URL here.

mdjnelson · 2025-07-06T11:34:32Z

element/qrcode/classes/element.php

            $urlparams = [
-                'code' => $code,
-                'qrcode' => 1,
+                'certId' => $code,


Why are we changing the name of the variable?

@fulldecent our verification URL is like "https://app.pacificmedicaltraining.com/verify?certId=1069-9865-6509", can we replace certId to code, then we won't require this change in the $urlprams.

mdjnelson · 2025-07-06T11:35:38Z

settings.php

    'customcert/codegenerationmethod',
    get_string('codegenerationmethod', 'customcert'),
    get_string('codegenerationmethod_desc', 'customcert'),
-    0, // Default option (0 = Upper/lower/digits random string method).


Why remove the '.'?

mdjnelson · 2025-07-06T11:35:50Z

settings.php

    [
-        0 => get_string('codegenerationmethod_upperlowerdigits', 'customcert'), // Upper/lower/digits random string.
-        1 => get_string('codegenerationmethod_digitshyphens', 'customcert'), // Digits with hyphens numeric code.
+        0 => get_string('Upper/lower/digits', 'customcert'), // Upper/lower/digits random string


Why do we need different language strings?

mdjnelson · 2025-07-06T11:55:50Z

Apologies. Initially I said I would address these but there are a few uncertainties about this patch. I would like this cleaned up so I can see a clear solution (right now it's touching code all over the place and im not sure why) at what is trying to be achieved. Please let me know when you are ready for me to re-review. Thanks.

Raza403 · 2025-07-08T17:07:16Z

Hi @mdjnelson, my apologies for bothering you, I have updated the title of this PR to [WIP], We'll let you know and will invite you to review this PR once this is ready.

mdjnelson · 2025-07-09T09:30:27Z

db/install.xml

      <INDEXES>
        <INDEX NAME="userid-customcertid" UNIQUE="false" FIELDS="userid, customcertid"/>
-        <INDEX NAME="code" UNIQUE="false" FIELDS="code"/>
+        <INDEX NAME="code" UNIQUE="true" FIELDS="code"/>


This has to be false due to backup/restore issues (which are always a pain cause I almost always forget abt it).

mdjnelson · 2025-07-09T09:30:52Z

db/upgrade.php

        upgrade_plugin_savepoint(true, 2024042210, 'mod', 'customcert');
    }

+    if ($oldversion < 2025041401) {


This upgrade step was part of another PR, so can be removed.

fixed at 8876d20

As per mdjnelson#681 (review)

… being generated. This PR is made for #1 and #2 custom cert, updated the names of both code generation methods. Added view_user_cert page, a logged in user can see certificate if having a valid token and ecard code. This is being used from view_tokens.php page. customcert view certificate function is made. This is made for mdjnelson#212 Updated verify certificate URL. This PR is made for 221 Updated the verify URL for QR code.

…is made for mdjnelson#231

As per mdjnelson#681 (review)

Raza403 · 2025-08-05T18:20:42Z

@fulldecent @mdjnelson this is passing all checks and is working. This PR is good to review.

fulldecent · 2025-08-11T19:33:29Z

view_user_cert.php

+require_once($CFG->dirroot . '/mod/customcert/lib.php');
+
+// Allows guest access to course with ID 1.
+require_login(1, false);


Does this line cause this script to only work if the site is configured with "allow guest login" ?

Are there any other situations where this line of code would cause the certificate to not be displayed?

Hard-coding the front-page course (ID 1) forces everyone to “log in to course 1 (guest if allowed)” even though this page isn’t about course 1 at all. It can bypass the actual course/module access rules for the certificate and behave unpredictably if guest access to the front page is disabled. Please look at https://github.com/mdjnelson/moodle-mod_customcert/blob/MOODLE_500_STABLE/verify_certificate.php to see how we handle non-users to access the site.

indicozy · 2025-08-23T01:51:32Z

I've tried this PR, everything works great. However, is it possible to add option to not require token if option All can verify certificate is set true?

fulldecent · 2025-08-23T05:17:08Z

Well every certificate has a URL. And the URL has a token. That's the official public URL of the certificate. I don't think it makes sense to have a separate, additional public URL for certificates if a specific setting is turned on.

mdjnelson · 2025-08-24T13:48:43Z

Looking now. Sorry for delay.

mdjnelson · 2025-08-24T14:07:35Z

classes/certificate.php

-        } while ($DB->record_exists('customcert_issues', ['code' => $code]));
-        return $code;
+        // If the upper/lower/digits is selected (0), use the upper/lower/digits code generation method.
+        if ($method == 0) {


I am not a fan of using a hard-coded variable like 0. It doesn't mean much to the developer using it.

mdjnelson · 2025-08-24T14:09:08Z

db/upgrade.php

        upgrade_plugin_savepoint(true, 2024042210, 'mod', 'customcert');
    }

-    if ($oldversion < 2024042213) {


We need an upgrade clause to remove these settings from the Moodle DB if it exists, probably use something like unset_config() iirc. Please add another upgrade step to do this and just leave this as is.

mdjnelson · 2025-08-24T14:09:48Z

classes/certificate.php

     *
     * @return string
     */
    private static function generate_code_digits_with_hyphens(): string {


Can you tell me why we are editing this function?

mdjnelson · 2025-08-24T14:11:48Z

view_user_cert.php

+ * @license    http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
+ * @param string $message The error message to display.
+ */
+function display_error_page($message) {


Please remove this and any usages of it, we can rely on Moodle's own error handling. This makes it forward compatible if this ever changes.

mdjnelson · 2025-08-24T14:18:07Z

view_user_cert.php

+require_once($CFG->dirroot . '/mod/customcert/lib.php');
+
+// Allows guest access to course with ID 1.
+require_login(1, false);


Hard-coding the front-page course (ID 1) forces everyone to “log in to course 1 (guest if allowed)” even though this page isn’t about course 1 at all. It can bypass the actual course/module access rules for the certificate and behave unpredictably if guest access to the front page is disabled. Please look at https://github.com/mdjnelson/moodle-mod_customcert/blob/MOODLE_500_STABLE/verify_certificate.php to see how we handle non-users to access the site.

mdjnelson · 2025-08-24T14:22:33Z

view_user_cert.php

+// The template defines the layout and content of the generated certificate.
+$template = $DB->get_record('customcert_templates', ['id' => $certificate->templateid]);
+if (!$template) {
+    display_error_page('The certificate template could not be found. Please contact the site administrator for assistance.');


This can be removed. If you perform a get_record call and pass MUST_EXIST then an exception is thrown and we want that.

mdjnelson · 2025-08-24T14:22:55Z

view_user_cert.php

+    $template->generate_pdf(false, $issue->userid);
+} catch (Exception $e) {
+    // Catch any errors that may occur while generating the certificate PDF.
+    display_error_page('Error generating certificate PDF. '


Remove this catch please.

mdjnelson · 2025-08-24T14:23:13Z

lib.php

+ * @param string $certcode The unique code of the certificate.
+ * @return string The generated public URL for the certificate.
+ */
+function generate_public_url_for_certificate(string $certcode): string {


This doesn't seem to be used anywhere.

mdjnelson · 2025-08-24T14:23:56Z

lib.php

+ * @param string $certcode The unique certificate code.
+ * @return string The generated HMAC signature.
+ */
+function calculate_signature(string $certcode): string {


The lib.php file is for functions that Moodle will call itself. We don't need to put it there. I would suggest creating a new class in the classes folder for this functionality.

mdjnelson · 2025-08-24T14:24:20Z

lib.php


-    if (($user->id != $USER->id)
-            && !has_capability('mod/customcert:viewallcertificates', context_system::instance())) {
+    if (


This looks worse than before. Was this change necessary?

Raza403 force-pushed the features/custom-cert-view-all-certificates#231 branch from b215843 to f478725 Compare June 24, 2025 10:17

mdjnelson reviewed Jul 6, 2025

View reviewed changes

Raza403 changed the title ~~Updated view_user_cert to support all certificates.~~ [WIP] Updated view_user_cert to support all certificates. Jul 8, 2025

mdjnelson reviewed Jul 9, 2025

View reviewed changes

fulldecent added a commit to modern-training-solutions/moodle-mod_customcert that referenced this pull request Jul 14, 2025

Update upgrade.php

8876d20

As per mdjnelson#681 (review)

Raza403 and others added 9 commits July 27, 2025 21:10

Updated view_user_cert to view all certs including CE certs. This PR …

d17d5f2

…is made for mdjnelson#231

removed some errors in the code

d4ed4eb

removed some errors in the code

9fa0eac

Update upgrade.php

288d0f5

As per mdjnelson#681 (review)

customcert fixed errors.

2728a48

Fixed errors.

129747f

Fixed errors

b60c152

Fixed errors

9b6ab3a

Raza403 force-pushed the features/custom-cert-view-all-certificates#231 branch from fd94167 to 9b6ab3a Compare July 27, 2025 16:11

fixed errors

8fd2bf3

Raza403 added 4 commits August 5, 2025 19:12

custom_cert fixed the phpdoc errors.

820882b

Allowed guest access.

20e732e

Fixed an error in lib.php, to prevent direct access

6cdab98

custom cert fixed an error

cce2c41

Raza403 changed the title ~~[WIP] Updated view_user_cert to support all certificates.~~ Updated view_user_cert to support all certificates. Aug 6, 2025

fulldecent mentioned this pull request Aug 11, 2025

[docs] Allow pages to not use require_login moodle/devdocs#1416

Open

fulldecent reviewed Aug 11, 2025

View reviewed changes

indicozy mentioned this pull request Aug 23, 2025

Add wsfunction API Endpoint to Retrieve Student Certificates #711

Closed

mdjnelson reviewed Aug 24, 2025

View reviewed changes

Updated view_user_cert to support all certificates. #681

Are you sure you want to change the base?

Updated view_user_cert to support all certificates. #681

Uh oh!

Conversation

Raza403 commented May 1, 2025

Uh oh!

mdjnelson commented May 1, 2025

Uh oh!

mdjnelson commented Jun 23, 2025

Uh oh!

Raza403 commented Jun 24, 2025

Uh oh!

mdjnelson commented Jul 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mdjnelson commented Jul 6, 2025

Uh oh!

Raza403 commented Jul 8, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Raza403 commented Aug 5, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

indicozy commented Aug 23, 2025

Uh oh!

fulldecent commented Aug 23, 2025

Uh oh!

mdjnelson commented Aug 24, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mdjnelson commented Jul 6, 2025 •

edited

Loading