Move the WeBWorK::PG module to PG. by drgrice1 · Pull Request #1792 · openwebwork/webwork2

drgrice1 · 2022-08-20T11:03:14Z

Note that the WeBWorK::PG::Local and WeBWorK::PG::Remote modules have been removed. The Local variant is integrated directly into WeBWorK::PG, and the Remote variant was not working and is in disrepair.

Remove the pg special environment variables ALWAYS_SHOW_HINT_PERMISSION_LEVEL and ALWAYS_SHOW_SOLUTION_PERMISSION_LEVEL. Use the always_show_hint and always_show_solution permissions directly. Whether solutions and hints are shown is now controlled by webwork2 and not by pg.

The pg variable/flag $showHint is removed. When and if hints are shown is now controlled by the instructor with a combination of options at the course, set, and problem level.

Note that hints were not implemented in gateway quizzes and still aren't.

Also the options to use knowls for solutions and hints have been removed. Always use knowls for solutions and hints.

Showing the various pieces of debugging information for a problem is now controlled with translation options. Webwork permissions are converted into these debugging options. The VIEW_PROBLEM_DEBUGGING_INFO special pg environment variable was removed and is one of these debugging options.

Although the permissionLevel and effectivePermissionLevel are still passed in, they are no longer used by anything in PG. A translator option named isInstructor is passed into the environment that can be set to get instructor things to happen. Another special translator option for scaffolds named forceScaffoldsOpen can be passed to force all scaffolds to be open. This is set in the library browser and in gateway quizzes.

Reduced scoring is implemented on the webwork2 side. PG no longer does that. It is implemented with the old method where the reduced score is saved in the problem status and the score before the reduced scoring period begins is saved in the sub_status. This can be changed later to fix some of the other issues with reduced scoring.

Several other environment variables that are clearly not needed have also been removed including the appletPath and session key.

Note that PGalias no longer uses the course name, user login, set number, and problem number for generating resource UUID's. It now only uses the problem seed, psvn, and problemUUID. So webwork2 constructs a problemUUID that guarantees the necessary uniqueness.

This depends on openwebwork/pg#709.

drdrew42

Mostly inconsequential feedback from initial code review. Testing forthcoming.

conf/localOverrides.conf.dist

lib/FormatRenderedProblem.pm

lib/WeBWorK/ContentGenerator/Instructor/ProblemSetDetail.pm

drdrew42 · 2022-08-24T14:35:26Z

lib/WeBWorK/ContentGenerator/Instructor/ProblemSetList.pm

 			} elsif ( $item eq 'showMeAnother' ) {
 			    $showMeAnother = ( $value ) ? $value : 0;
+			} elsif ( $item eq 'showHintsAfter' ) {
+			    $showHintsAfter = ( $value ) ? $value : 0;


Suggested change

$showHintsAfter = ( $value ) ? $value : 0;

$showHintsAfter = ( $value ) ? $value : -2;

or, more ideally, pull the server default value?

Now I see, there's a check later that would override with the default value -- but in order for that to occur, the alternate value here must fail the regex /-?\d+/... so empty string?

Yeah, the logic does seem messed up here. All of the settings here should use the defaults already it seems.

In fact the later "problem value clean up" won't even occur if these do happen. This is all in a big if .. elsif ... elsif statement.

drdrew42 · 2022-08-24T14:43:27Z

lib/WeBWorK/ContentGenerator/Instructor/ProblemSetList.pm

 			    $showMeAnother =~ s/[^-?\d-]*//g;

+			    unless ($showHintsAfter =~ /-?\d+/) {$showHintsAfter = $showMeAnother_default;}
+			    $showHintsAfter =~ s/[^-?\d-]*//g;


Suggested change

$showHintsAfter =~ s/[^-?\d-]*//g;

$showHintsAfter =~ s/[^-\d]*//g;

don't allow ? and remove duplicate - (this should happen in all these regex expressions...)

It looks like there is another issue on line 2182, it should be falling back to $showMeAnother_default and not to $showMeAnother_default.

You caught me on a cut and paste without real thought on this one.

Yeah, I am not sure what was intended with the regular expressions including the question mark there. Did some errant values have question marks at some point? It is also debatable if these substitutions are really at all effective in any case. If there is a value that the substitutions actually do something for, that means the administrator has the defaults set to something invalid. In addition this won't fix all bad values. One thing the substitutions wouldn't fix is a value like '5-3'.

Agreed -- it looks like someone threw [] around the earlier regex... dunno why the - ended up in there a second time. I'd lean towards removing all of these secondary regexes.

drdrew42 · 2022-08-24T15:35:06Z

lib/WeBWorK/Utils/ProblemProcessing.pm

+# WARNING: The usage of $self throughout this file is incorrect and quite misleading.  In all cases $self needs to at
+# least be a WeBWorK::ContentGenerator object even.  In addition it must be ensured that the $self object has the
+# correct hash values to work with the method.


Suggested change

# WARNING: The usage of $self throughout this file is incorrect and quite misleading. In all cases $self needs to at

# least be a WeBWorK::ContentGenerator object even. In addition it must be ensured that the $self object has the

# correct hash values to work with the method.

# WARNING: The usage of $self throughout this file is incorrect and quite misleading. In all cases $self needs to at

# least be a WeBWorK::ContentGenerator object. In addition it must be ensured that the $self object has the

# correct hash values to work with the method.

Is it worth performing ref($self) =~ /WeBWorK::ContentGenerator/ check in these subroutines? I realize that's not entirely enough... necessary, but not sufficient.

Perhaps. The old code had comments about what the ref should be, but no actual check. I will leave this for future thought and work. The design of the old ProblemUtil/ProblemUtil.pm module was rather flawed, and this improves things a little, but more needs to be done.

lib/WeBWorK/Utils/ProblemProcessing.pm

drgrice1 · 2022-09-01T14:00:41Z

I tested reduced scoring with gateway quizzes, and things are working correctly with that.

I also added a check to ensure that the recorded score is never lowered by webwork regardless of what comes back from PG.

Note that the WeBWorK::PG::Local and WeBWorK::PG::Remote modules have been removed. The Local variant is integrated directly into WeBWorK::PG, and the Remote variant was not working and is in disrepair. Remove the pg special environment variables ALWAYS_SHOW_HINT_PERMISSION_LEVEL and ALWAYS_SHOW_SOLUTION_PERMISSION_LEVEL. Use the always_show_hint and always_show_solution permissions directly. Whether solutions and hints are shown is now controlled by webwork2 and not by pg. The pg variable/flag $showHint is removed. When and if hints are shown is now controlled by the instructor with a combination of options at the course, set, and problem level. Note that hints were not implemented in gateway quizzes and still aren't. Also the options to use knowls for solutions and hints have been removed. Always use knowls for solutions and hints. Showing the various pieces of debugging information for a problem is now controlled with translation options. Webwork permissions are converted into these debugging options. The VIEW_PROBLEM_DEBUGGING_INFO special pg environment variable was removed and is one of these debugging options. Although the permissionLevel and effectivePermissionLevel are still passed in, they are no longer used by anything in PG. A translator option named isInstructor is passed into the environment that can be set to get instructor things to happen. Another special translator option for scaffolds named forceScaffoldsOpen can be passed to force all scaffolds to be open. This is set in the library browser and in gateway quizzes. Reduced scoring is implemented on the webwork2 side. PG no longer does that. It is implemented with the old method where the reduced score is saved in the problem status and the score before the reduced scoring period begins is saved in the sub_status. This can be changed later to fix some of the other issues with reduced scoring. Several other environment variables that are clearly not needed have also been removed including the appletPath and session key. Note that PGalias no longer uses the course name, user login, set number, and problem number for generating resource UUID's. It now only uses the problem seed, psvn, and problemUUID. So webwork2 constructs a problemUUID that guarantees the necessary uniqueness.

unreduced score in to PG for the recorded score. Otherwise the recorded score can be updated incorrectly. This is determined simply by checking if the sub_status is less that the status in the case that reduced scoring is enabled for the set. Also, a check is added that the score returned by PG (and reduced if reduced scoring is in effect) is less than the currently saved score. If that is the case, the the score in the database is not changed.

drgrice1 force-pushed the pg-envir-work branch from 1849a4f to dc4bd5d Compare August 21, 2022 12:44

drdrew42 reviewed Aug 24, 2022

View reviewed changes

drgrice1 force-pushed the pg-envir-work branch from 8409960 to 8b6a22a Compare August 24, 2022 21:27

pstaabp approved these changes Aug 30, 2022

View reviewed changes

drgrice1 force-pushed the pg-envir-work branch 2 times, most recently from 6083f3f to acf5fdc Compare September 1, 2022 11:47

drgrice1 force-pushed the pg-envir-work branch 3 times, most recently from 64ee2a0 to d9ce8ab Compare September 9, 2022 17:53

drgrice1 force-pushed the pg-envir-work branch 2 times, most recently from f132368 to 22d0d48 Compare September 14, 2022 10:53

pstaabp force-pushed the pg-envir-work branch from 990f80e to d31cff1 Compare September 19, 2022 10:49

drgrice1 force-pushed the pg-envir-work branch from d31cff1 to 038ba92 Compare September 28, 2022 12:14

drgrice1 added 4 commits October 11, 2022 10:52

Add changes from reviews.

b280b5b

Remove the now unused checkurl external program.

dc4c86a

drgrice1 force-pushed the pg-envir-work branch from 038ba92 to dc4c86a Compare October 11, 2022 15:52

pstaabp merged commit 2d5fc58 into openwebwork:develop Oct 12, 2022

drgrice1 mentioned this pull request Oct 13, 2022

Show set header in proctored login. #1810

Merged

drgrice1 deleted the pg-envir-work branch October 13, 2022 15:35

drgrice1 mentioned this pull request Oct 16, 2022

Fix an issue in gateway quizzes that causes correct answers to be displayed on all submissions. #1812

Merged

	$showHintsAfter = ( $value ) ? $value : 0;
	$showHintsAfter = ( $value ) ? $value : -2;

	$showHintsAfter =~ s/[^-?\d-]*//g;
	$showHintsAfter =~ s/[^-\d]*//g;

Uh oh!

Conversation

drgrice1 commented Aug 20, 2022

Uh oh!

drdrew42 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

drgrice1 commented Sep 1, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants