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

New feature: Add default value at the global level/plugin's settings page. #14

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

Conversation

sumaiyamannan
Copy link

Hi,
This will create a settings page for the plugin to save the default value of honestycheckrequired.
The new instances of a quiz will have this default value set at the time of creation.

Regards,
Sumaiya

@sumaiyamannan sumaiyamannan marked this pull request as draft November 1, 2023 04:00
@sumaiyamannan sumaiyamannan marked this pull request as ready for review November 1, 2023 04:01
Copy link
Member

@timhunt timhunt left a comment

Choose a reason for hiding this comment

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

This is a good change, in principle. Thanks. However, it needs to be implemented consistently with other similar quiz settings, and the implementation needs to be technically right before it can be merged.

rule.php Outdated Show resolved Hide resolved
rule.php Outdated
@@ -86,22 +86,19 @@ public static function add_settings_form_fields(
0 => get_string('notrequired', 'quizaccess_honestycheck'),
1 => get_string('honestycheckrequiredoption', 'quizaccess_honestycheck'),
));
$default = get_config('quizaccess_honestycheck', 'honestycheckrequired');
$mform->setDefault('honestycheckrequired', $default);
Copy link
Member

Choose a reason for hiding this comment

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

What is the point of the lcoal variable $default here? Much better to put the $default inline in the next line.

settings.php Outdated Show resolved Hide resolved
settings.php Outdated

$settings->add(new admin_setting_configselect(
'quizaccess_honestycheck/honestycheckrequired',
get_string('honestycheckrequired', 'quizaccess_honestycheck'),
Copy link
Member

Choose a reason for hiding this comment

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

In settings.php, you shoudl use new lang_string, not get_string, for performance reasons.

settings.php Outdated
'quizaccess_honestycheck/honestycheckrequired',
get_string('honestycheckrequired', 'quizaccess_honestycheck'),
get_string('honestycheckrequired', 'quizaccess_honestycheck'),
1,
Copy link
Member

Choose a reason for hiding this comment

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

Making the default 1 might be what you want, but you are contributing to an open source project. For most people, the default of 0 is more appropriate, so we should set that.

settings.php Outdated
0 => get_string('notrequired', 'quizaccess_honestycheck'),
1 => get_string('honestycheckrequiredoption', 'quizaccess_honestycheck')
)
));
Copy link
Member

Choose a reason for hiding this comment

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

We should be consistent with what is done for all the other quiz settings, and give the admin control over whether the setting is advanced or locked using

    $setting->set_advanced_flag_options(admin_setting_flag::ENABLED, false);
    $setting->set_locked_flag_options(admin_setting_flag::ENABLED, false);

(Look at mod/quiz/settings.php if you don't understand what i mean.)

Copy link
Author

Choose a reason for hiding this comment

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

I have fixed this as well

@sumaiyamannan
Copy link
Author

Hi Tim,
Thank you for your feedback. I have worked on it plus some additional syntax errors that I see were picked from the actions workflow. I have done a forced push so there is a single commit for you to merge. Kindly have a look and let me know if any more changes are needed to be done.
Regards,
Sumaiya

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