Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Custom setting additional date to customize the date display #463

Open
wants to merge 1 commit into
base: MOODLE_311_STABLE
Choose a base branch
from

Conversation

DinhHien0307
Copy link

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

@@ -178,5 +178,46 @@ function xmldb_customcert_upgrade($oldversion) {
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

@@ -305,11 +305,12 @@ public static function get_date_formats() {
$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.

@@ -178,5 +178,46 @@ function xmldb_customcert_upgrade($oldversion) {
upgrade_mod_savepoint(true, 2020110901, 'customcert');
}

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