Merge upstream WeBWorK 2.20 into UBC branch#52
Open
ltdevopsbot[bot] wants to merge 1404 commits into
Open
Conversation
There is no reason to use javascript for this. Just make the links disabled to begin with. Furthermore, there is no reason to use jQuery for this. I thought all webwork2 jQuery usage had already been removed, but this was missed. To test this create a JITAR set with two or more problems (or really two or more nested sets of problems), and set "Restrict Problem Progression" to "Yes". Assign the set to a student user, and then log in as that student user. On the problem list page the latter prolems in the set should be grayed out, and clicking on them does nothing (that already works without javascript because these aren't even `a` tags, they are `span` tags). If you open the first problem, then the latter problems in the prroblems list in the site menu should be grayed out, and clicking on them does nothing. Currently, that uses jQuery javascript to disable those links. With this pull request, they are just disabled to begin with.
Check for LMS ID first when authenticating from LMS
the active %userRoles hash. Will prevent Accounts Manager from failing. See openwebwork#2625 but when editing a user with such a value, will only allow choosing a defined level.
The crypt function cannot handle most wide (Unicode) characters, but does handle some by attempting to downgrade to an 8-bit string. In order to support wide characters in passwords while not breaking any passwords which worked before this change, a password will be encoded into UTF-8 before being sent to crypt only when crypt dies on the original string.
This makes it easy to generate a password hash or verify a given password against a previously generated password hash. Note that this uses Encode::encode (via the Mojo::Util encode method) rather than Encode::utf8_encode. The Encode::utf8_encode method should not be used anymore.
…-extensions adjustments to date extension achievement items
There is really no production scenario where this should be anything but 1. If you are really serving without https, then you can change this. And good luck dealing with your users having to click through the warnings to get to your site. Note that this just works if you are using `localhost` in development also. Browsers have a built in exception for `localhost` and consider it secure without actual certificates. I am tired of seeing forum issues because system administrators forget or don't know to change this.
Change the default value of `$CookieSecure` to 1.
…-tweaks Add support for wide characters in passwords. (replacement of openwebwork#2628)
…lem-no-jquery Make JITAR disabled problems disabled without javascript.
This adds achievement items that modify the reduced scoring time period while leaving other settings alone. These are designed for classes which use reduced scoring and keep assignments open in the reduced scoring period for a long time (such as the end of the course). + ExtendReducedDate -> Extends the reduced scoring date 24 hours. + SuperExtendReducedDate -> Extends the reduced scoring date 48 hours. + NoReducedCred -> Removes the reduced scoring flag on an assignment to allow full credit after the reduced scoring date.
…levels Support for custom permission levels not in the userRoles hash (replacement of openwebwork#2629)
This way the parameters can be sorted in this one instance where this is desired.
The `formatRenderedProblem` method of the `FormatRenderedProblem` module ensures that the `debug_messages` key of the rendered problem result is an array reference, but checks the wrong thing. It checks if the result itself is an array reference instead of the `debug_messages` hash key of the result. Since the result is always a hash reference and never an array reference, this means that the debugging messages from PG are always wiped out and replaced with a reference to an empty array. This was discovered when investigating openwebwork#2661. If we want to switch the PG problem editor to using the `debug` output format of the `formatRenderedProblem` method we could, but in order to get the benefit of that change we need this.
Construct the emailable URL directly and don't use systemLink for that.
…ent-items Add achievement items that modify reduced scoring.
Add leaderboard for achievements.
Updates to grading tests for other users.
make only outer edge of achivements images transparent and fix a miss…
…red-debug-messages Fix debugging messages in the `formatRenderedProblem` method.
This is as discussed in openwebwork#2661, openwebwork#2662 and the developer meeting. The PG problem editor is switched to the debug format from the simple format. The only difference for the problem editor page is that if there are debug messages they are now shown. Usually there aren't any such messages, and so there isn't a difference. But if something is enabled like the MathObject diagnostics cmp flag, then those debug messages are shown.
This makes it so that persistence hash is only saved to the database when answers are submitted. In the case that answers can be checked by the user (via the "Check Answers" button), then the persistence hash is saved JSON encoded in a hidden form field. That case does not need the security of saving to the database, and in that case it shouldn't be saved to the database because answers aren't being recorded and the data in that case should temporary. So a hidden form field is just right for this. I objected to the previous implementation when openwebwork#1940 was submitted, but allowed it at that time, but this is how the problem_data should have been implemented. Nothing should be written to the database for this when answers are not being recorded. If an instructor is acting as a student, the previous code was saving the persistent data to the student's problem even when the instructor just enters the problem. Of course it was also saving every time the instructor did anything with the problem including using the "Preview My Answers" button, the "Check Answers" button, the "Show Correct Answers" button, the "Show Problem Grader" button. Literally any form submission of the page. That just was not thought out. The render_rpc (and html2xml) routes use the hidden form field approach to also support saving the problem data from the persistence hash. This means that problems that use the persistence hash can be tested in the PG problem editor. Note that the PERSISTENCE_HASH_UPDATE flag is removed. That didn't improve efficiency at all. The processing that was done with that was too much. Even if this were saved when it was before it would have been more efficient to just save it. The handling of the persistence hash is also reworked for PG in a corresponding pull request. PG just sends the hash, and it can contain anything that can be JSON encoded. Webwork2 just JSON encodes it and stores it, but only when answers are submitted.
Different dates can have different timezone adjustments in the case that the client timezone and server timezone are different and one of those time zones adheres to daylight savings time and the other does not. For example if the client timezone is America/Phoenix and the server timezone is America/Chicago. The America/Phoenix timezone does not adhere to daylight savings time and is always UTC-7 while America/Chicago is UTC-5 during daylight savings time, but is UTC-6 during standard time. So if an instructor is in the America/Phoenix timezone, but working on a server set for the America/Chicago timezone and selects a date that is during daylight savings time, then the current code use an incorrect differential of 1 hour because it uses the same differential for all dates. So this computes the timezone adjustment for a given date so that the correct timezone differential for that time is used. This fixes issue openwebwork#2654.
Switch the problem editor to use the debug format.
change introduction to feedback emails
…update_18612f0efb Updates for file lib/WeBWorK/Localize/webwork2.pot in ru_RU [Manual Sync]
…update_426aa555ba Updates for file lib/WeBWorK/Localize/webwork2.pot in de [Manual Sync]
…update_69a309b01c Updates for file lib/WeBWorK/Localize/webwork2.pot in cs_CZ [Manual Sync]
…update_19919ae7d9 Updates for file lib/WeBWorK/Localize/webwork2.pot in zh_HK [Manual Sync]
…update_b66286f39c Updates for file lib/WeBWorK/Localize/webwork2.pot in zh_CN [Manual Sync]
…update_2c8dfe77ba Updates for file lib/WeBWorK/Localize/webwork2.pot in he_IL [Manual Sync]
…update_57f4225881 Updates for file lib/WeBWorK/Localize/webwork2.pot in es [Manual Sync]
…update_e78b881df7 Updates for file lib/WeBWorK/Localize/webwork2.pot in hu [Manual Sync]
…update_6cddca662b Updates for file lib/WeBWorK/Localize/webwork2.pot in fr_CA [Manual Sync]
…update_97ac41b207 Updates for file lib/WeBWorK/Localize/webwork2.pot in el [Manual Sync]
…update_d23ae80e68 Updates for file lib/WeBWorK/Localize/webwork2.pot in ko [Manual Sync]
…update_9cdac29bdb Updates for file lib/WeBWorK/Localize/webwork2.pot in fr [Manual Sync]
Currently if the session has expired and one of the options in the "Format Code" tabe is used in the PG problem editor, then the message reads `Error: can't access property "status", t.result_data is undefined` which is not very informative. This makes it so that the error message in the response which is `Error: Authentication failed. Log in again to continue.` An easy way to test this is to open a problem in the problem editor, delete the cookie for the session in the developer tools, and then switch to the "Format Code" tabe and click the "Format Code" button.
…xpired-issue Fix the session expired message in the problem editor.
record/update user lis_source_did even when grade passback not in use
WeBWorK 2.20 Release Candidate
# Conflicts: # .github/workflows/linter.yml # conf/database.conf.dist
xcompass
requested changes
Jun 19, 2026
xcompass
left a comment
xcompass
left a comment
Member
There was a problem hiding this comment.
Review: REQUEST CHANGES — semantic regression in UBC's LTI/job integration
This is a vendor merge of upstream WeBWorK-2.20 (~1400 commits) into ubc. The automated checks (ancestry, no conflict markers, bash syntax) pass, but they only detect textual problems. Verified locally against merge head 92bbe13, base origin/ubc, and tag WeBWorK-2.20, the merge silently breaks UBC's LTI 1.3 and DelayedJob features.
Root cause: the conf/database.conf.dist conflict was resolved by keeping a file upstream deleted
Upstream 2.20 moved the DB layout out of conf/database.conf.dist (deleted) and into lib/WeBWorK/DB/Layout.pm. This was a modify/delete conflict, resolved by keeping UBC's database.conf.dist. But nothing includes it anymore:
- Base
ubcloaded the layout viaconf/defaults.config:include("./conf/database.conf.dist")+*dbLayout = $dbLayouts{...}(basedefaults.config:562/576). - Merged
defaults.configdropped that include, andlib/WeBWorK/DB.pm:161now builds the layout withdatabaseLayout($ce->{courseName})(upstream'sWeBWorK::DB::Layout).
So conf/database.conf.dist is now orphaned dead config, and the active layout = upstream's Layout.pm.
CRITICAL — UBC LTI 1.3 (AGS/NRPS, Canvas, Moodle) crashes at runtime
Layout.pm registers only lti_launch_data + lti_course_map. UBC's LTI tables — lti_resource_link, lti_user, lti_contexts, lti_nonces, lti_access_tokens (WeBWorK::DB::Record::LTI1p3::*) — exist only in the orphaned file, so DB.pm never creates handles for them. UBC's accessor methods survived the merge (DB.pm:2113 getLTIContext, 2279 getLTIResourceLink, 2335 getLTIUser) and dereference $self->{lti_contexts} / {lti_resource_link} handles that are never instantiated. The launch path calls them:
lib/LTI1p3/Entrypoint/Launch.pm:104,237→$db->getLTIContext(...)lib/LTI1p3/Entrypoint/Launch.pm:269,271→$db->getLTIResourceLink/newLTIResourceLink(...)
The app boots, but an LTI launch dereferences an undefined table handle → runtime crash.
HIGH — UBC DelayedJob background jobs break the same way
funcmap, job, note, error, exitstatus (WeBWorK::DB::Record::DelayedJob::*) also live only in the orphaned file (upstream uses Minion). UBC's job code is exercised: lib/DelayedJob/Service.pm dispatches PushUserGrades / PushClassGrades / GetClassMembership / SendCaliperEvent, invoked from lib/WeBWorK/Utils/ProblemProcessing.pm:224 and lib/Caliper/Sensor.pm:59. Grade push-back and Caliper events will fail.
HIGH — No Perl/runtime/test validation was run
The PR notes Perl checks couldn't run (missing Mojo::Base). A merge this size, with a modify/delete resolution, must boot in the Docker/CI env and pass the test suite plus a real LTI launch before merge.
MEDIUM
- Two parallel LTI implementations now coexist (UBC
LTI1p3::*vs upstreamLTILaunchData/LTICourseMap). Decide which is authoritative for UBC. .github/workflows/linter.ymlconflict kept UBC's older config (perl:5.34,Perl::Tidy@20220613,branches-ignore: ubc) — intentional, but the PR isn't linted to 2.20 standards. Please confirm.
Required before merge
- Decide LTI strategy (adopt upstream 2.20 LTI vs. keep UBC's LTI1p3) and reconcile the layout in
lib/WeBWorK/DB/Layout.pmaccordingly — register UBC's tables there, or port the AGS/NRPS code onto upstream's schema. - Decide job strategy (Minion vs. DelayedJob) and register/remove tables to match.
- Remove or properly re-wire the orphaned
conf/database.conf.dist. - Boot a test instance and run the suite + a real LTI launch (Canvas and Moodle) before merging.
Note: the bot's checks can't catch any of this — there's no textual conflict because UBC's and upstream's LTI live in differently-named files, so both were kept silently. Consider adding a "boots + tests pass + LTI smoke test" gate to the merge bot.
Upstream 2.20 moved the database layout from conf/database.conf.dist into lib/WeBWorK/DB/Layout.pm and deleted database.conf.dist. The 2.20 merge kept UBC's database.conf.dist, but the merged defaults.config no longer includes it and DB.pm now builds the layout from databaseLayout(). As a result UBC's LTI Advantage and DelayedJob tables were never registered, so LTI launches (getLTIContext/getLTIResourceLink) and DelayedJob grade sync would dereference undefined table handles and crash at runtime. Re-register UBC's tables in databaseLayout(), translated to the 2.20 entry shape, alongside upstream's lti_launch_data/lti_course_map (different tables): - lti_resource_link, lti_user, lti_contexts, lti_nonces, lti_access_tokens - funcmap, job, note, error, exitstatus Remove the now-orphaned conf/database.conf.dist and fix the stale reference to it in conf/site.conf.dist.
Member
Pushed a fix for the LTI/DelayedJob DB-layer regression (7c7e14f)This addresses the CRITICAL + HIGH findings from my review. Strategy chosen: keep UBC's LTI 1.3 authoritative (minimal change), leaving upstream's What the commit does:
Validation done:
Still required before merge (the validation HIGH finding):
Not in scope here: migrating to / removing upstream's LTI stack. The two stacks coexist (different tables and URLs); consolidating them is a separate follow-up if desired. |
…validation
Two regressions the 2.20 merge introduced with no textual conflict, so they
passed the automated checks:
1. verify(): the merge kept UBC's use of $log_error but dropped its
`my $log_error = $self->{log_error};` declaration. Under `use strict` the
undeclared $log_error is a compile-time fatal, so WeBWorK::Authen fails to
load and all authentication breaks. Restore the declaration.
2. killSession(): the merge dropped upstream 2.20's
`delete $c->stash->{'webwork2.database_session'};`. In 2.20, store_session()
(run in after_dispatch) re-persists the session key unless that stash entry
is cleared, so logout was re-saving the session instead of invalidating it.
Restore the cleanup so logout deletes the server-side session.
Verified with a strict-compile check: the pre-fix verify() fails with
"Global symbol \$log_error requires explicit package name"; both subs compile
cleanly after the fix.
Member
Pushed fixes for two CRITICAL merge regressions (2881efa)Both were introduced by the 2.20 merge with no textual conflict, so they passed the automated checks. 1. Proof (strict-compile check of the merged file before the fix): 2. Both subs now compile cleanly under Still open from the second-pass review (not addressed here)
Strong recommendationAdd |
…n config
Four critical regressions from the upstream 2.20 merge - all merged
cleanly with no textual conflict, none could boot or run:
- conf/defaults.config: drop the dead $ConfigValues block and its fatal
include('conf/LTIConfigValues.config'). 2.20 deleted that file (config
values now come from lib/WeBWorK/ConfigValues.pm), so the include made
every CourseEnvironment->new die and nothing could boot.
- conf/defaults.config: rename $sessionKeyTimeout to $sessionTimeout
(the 2.20 name; Authen.pm reads sessionTimeout with no fallback, so
undef made every session expire one second after creation).
- Pass $ce to WeBWorK::DB->new at 12 call sites (LTI 1.3 entrypoints,
SAML2 ACS controller, DelayedJob workers, cron scripts). 2.20 removed
$ce->{dbLayout} and DB->new now takes the course environment; passing
the old key crashed every LTI/SAML2 login and all grade sync.
- Retarget WeBWorK::Utils imports after the 2.20 Utils split
(Utils::DateTime/Files/JITAR/Sets) in 7 files that croaked at compile
time; swap formatDateTime args for the new (time, format, timezone)
order; export UBC's 5-return grade_set variant from WeBWorK::Utils
(with its moved jitar/after helpers imported); point StudentProgress
at Utils::Sets for list_set_versions (Utils::Grades was deleted);
drop unused grade_*/readFile/runtime_use imports.
- Replace merge-mangled WeBWorK::Authen::Shibboleth (unclosed sub that
failed perltidy CI; neither parent's logic survived intact) with the
pre-merge UBC version, updated for the sessionTimeout rename.
Claude-Session: https://claude.ai/code/session_01TFNbNsmYakWLrkzqeWNY3C
Member
Review round 3: consistency of UBC changes × upstream 2.20 — 6 criticals found, fixed in c55b203Focus: do the recent Verified clean first
CRITICAL — all fixed in c55b203
Still open (not fixed here)
RecommendationThe fixes above are static-analysis-verified (stubbed |
- setmaker.js: set the alert toast title/message via textContent instead of interpolating into innerHTML (js/xss-through-dom, high). All callers pass plain text, so rendering is unchanged. - linter.yml, main.yml: limit the default GITHUB_TOKEN to contents: read (actions/missing-workflow-permissions). Neither workflow uses the token beyond checkout; Docker Hub pushes use dedicated secrets. Claude-Session: https://claude.ai/code/session_01TFNbNsmYakWLrkzqeWNY3C
One-time formatting pass with the CI-pinned Perl::Tidy version so the lint workflow inherited from upstream 2.20 passes. The ubc branch was deliberately exempted from this check (branches-ignore), so UBC files were never tidied; the PR-triggered run enforces it regardless. Formatting only - verified no semantic change (import/export cross-check, syntax checks, and spot checks of the earlier fix sites all pass). Claude-Session: https://claude.ai/code/session_01TFNbNsmYakWLrkzqeWNY3C
Perl::Tidy 20220613 does not converge in one pass on the nested reply->exception(maketext(...)) chain in Launch.pm and a boolean assignment in AssignmentAndGradeService.pm; CI runs against the once-tidied tree and found the second-pass delta. Verified stable under a further pass. Claude-Session: https://claude.ai/code/session_01TFNbNsmYakWLrkzqeWNY3C
Member
All CI checks now pass (b626507)Follow-ups since the round-3 fixes (c55b203):
Checks: perltidy ✅ CodeQL ✅ build ✅ Analyze ✅ Still open before merge (from the round-3 review): the two HIGHs — |
…oding
Two HIGH findings from the round-3 review:
- Restore bin/generate-OPL-set-def-lists.pl (byte-identical to the
pre-merge ubc version). Upstream 2.20 deleted it in favor of prebuilt
metadata-release artifacts, but docker-entrypoint.sh still invokes it
when the set-def JSON files are missing - under `set -eo pipefail` a
fresh-volume deploy exited 127 and the container aborted. Restoring
the script keeps startup self-contained (local generation, no network
or database dependency at that point in the entrypoint); all of its
dependencies (OPLUtils::writeJSONtoFile, $problemLibrary{root},
$contribLibrary{root}) still exist in 2.20.
- lib/Caliper/Sensor.pm: the merge replaced `use JSON` with Mojo::JSON
but left two `JSON->new->canonical->encode` calls, which would die on
the first Caliper event. Use JSON::PP (Perl core) instead - same
canonical (sorted-key) semantics, and unlike JSON.pm it is guaranteed
present now that 2.20 dropped the json packages from the docker build.
The Mojo::JSON encode_json import was otherwise unused.
Claude-Session: https://claude.ai/code/session_01TFNbNsmYakWLrkzqeWNY3C
Member
Both remaining HIGHs fixed (0209dd0) — CI still fully green
Checks on 0209dd0: perltidy ✅ CodeQL ✅ build ✅ Analyze ✅ Everything from the round-3 review is now closed except the documented mediums (dead |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Merges upstream openwebwork/webwork2 tag WeBWorK-2.20 into the UBC branch.
Verification performed locally:
Notes: