Skip to content

Conversation

DinhHien0307
Copy link

Hi @mdjnelson
I have created this pull request for issue #456.
Please take a look at it


upgrade_mod_savepoint(true, 2020110901, 'customcert');
}

Copy link
Author

Choose a reason for hiding this comment

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

This change I did to change all old items in DB which are saved in format '1', '2'... to new format

// Eg. 06/07/18 vs 6/07/18.
$date = 1530849658;

$suffix = self::get_ordinal_number_suffix(userdate($date, '%d'));
Copy link
Author

Choose a reason for hiding this comment

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

These changes are the new way I change the format date. User will add new settings like "=%d# %B, %Y", which = is always the first character and '#' to present for 1st, 2nd, 3rd as handling by this function get_ordinal_number_suffix, and should add an intro for this symbol in the description too.

]);
$DB->update_record('customcert_elements', $updateelement);
}}
$transaction->allow_commit();upgrade_mod_savepoint(true, 2021101100, 'customcert');
Copy link
Owner

Choose a reason for hiding this comment

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

These should be on a separate line.

}

if ($oldversion < 2021101100) {
$transaction = $DB->start_delegated_transaction();
Copy link
Owner

Choose a reason for hiding this comment

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

Don't need to put this into a transaction.

Copy link
Author

Choose a reason for hiding this comment

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

I think this improves performance as well as makes it more predictable

$records = $DB->get_records('customcert_elements', ['element' => 'date']);
$total = count($records);
$done = 0;
$pbar = new progress_bar('mod_customcert_change_formatdate', 500, true);
Copy link
Owner

Choose a reason for hiding this comment

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

No need for a progress bar as this is a quick change. I think all the code related to this progress bar can be removed. Thoughts?

Copy link
Author

Choose a reason for hiding this comment

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

If a server had a very large number of custom certificates, it's possible to take a long time so I think it makes sense to add a progress bar here. What do you think?

@DinhHien0307
Copy link
Author

Hi @mdjnelson,
I left some thoughts to discuss above, please take a look at them.

@mdjnelson mdjnelson force-pushed the MOODLE_311_STABLE branch 2 times, most recently from 49b746c to 8bc26b9 Compare January 10, 2022 09:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants