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

Refactor moodle test page. #144

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Refactor moodle test page. #144

wants to merge 5 commits into from

Conversation

xjiang-at-wiris
Copy link

@xjiang-at-wiris xjiang-at-wiris commented Jun 5, 2024

Description

Refactor of the moodle test page.
The result will be similar to this table (won't be the same, the most important is contains all information):
https://docs.google.com/spreadsheets/d/1hMj9gpHXNtPoB1Q9pAHOD2n-fefZYbtgyKcvgmoGh6U/edit#gid=866511881

Steps to reproduce

  1. Try with all supported Moodle version.
  2. Build and access http://localhost:8000/filter/wiris/info.php
  3. Check the page show no error and all tests works, and meet all conditions
    3.1. If some text are not updated, try purge cache in Site Administration
  4. Try to tweak parameters instrucction

taskid 43810

CHANGELOG.MD (filter)

refactor: Redesign test page. #43810

classes/pluginwrapper.php Show resolved Hide resolved
info.php Outdated Show resolved Hide resolved
Copy link
Contributor

@jgonzalez-at-wiris jgonzalez-at-wiris left a comment

Choose a reason for hiding this comment

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

@xjiang-at-wiris I checked the code and I think I will need your help to understand how these changes affect the test pages. On the other hand, code seems fine to me, to let's meet and test it out together ok? Thanks, great job! 💪🏾

info.php Show resolved Hide resolved
info.php Show resolved Hide resolved
@@ -40,14 +40,12 @@
$string['clearcachedesc'] = 'Clear MathType filter cache';
$string['clickwirisplugincorrectlyinstalled'] = 'Click the following button to test if MathType is correctly installed.';
$string['clickwirisquizzescorrectlyinstalled'] = 'Click the following button to test if WirisQuizzes is correctly installed.';
$string['contact'] = 'For more information or if you have any doubt contact WIRIS Support:';
$string['contact'] = 'For more information see <a href="https://docs.wiris.com/mathtype/en/mathtype-for-lms/mathtype-for-moodle.html">our documentation </a> or contact WIRIS Support:';
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this will conflict when changing the Moodle language. I'll check what happens when it is in another one and let you know!

Copy link
Author

Choose a reason for hiding this comment

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

Sure! I found that the language has a cache, so if the language is not updated, the cache needs to be purged. I will add this step in the "steps to reproduce." But how can we make users do it?

info.php Show resolved Hide resolved
@xjiang-at-wiris xjiang-at-wiris changed the base branch from stable to main June 7, 2024 09:57
info.php Outdated
Comment on lines 325 to 326
// Construct table
table_open();
Copy link

Choose a reason for hiding this comment

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

Suggested change
// Construct table
table_open();
// Construct HTML table
table_open();

Hablado por Slack, resuelta la confusión sobre que tabla trataba. Añadiendo que es una tabla HTML al comentario se tiene ese contexto.

@@ -213,22 +212,22 @@ function get_mt_filter_enabled($existsfilter){
return $filterenabled;
}

function get_exists_editor($editorname) {
function get_editor_exists_and_enabled($editorname) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe a clearer name ? Like editor_exists_and_enabled if the return is true or false. In my opinion if the name contains get I assume it returns something else than a bool

Copy link
Author

Choose a reason for hiding this comment

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

You’re right! But doing this would disrupt the all-get pattern... What does everyone think about that?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd leave the name as is, I agree with Ubaldo the name could be improved but in this context is not that important, and keeping the pattern seems more important in this case.

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.

None yet

5 participants