Conversation
somiaj
force-pushed
the
move-achievements-out-of-course
branch
2 times, most recently
from
September 22, 2026 02:04
813e73c to
31f20e5
Compare
System achievements should not be placed in a course. This way any changes to them will propagate to all courses using them. Instead courses should be able to create local overrides of system achievements as needed. This moves all of the default achievement evaluators and icons out of the modelCourse and places them in a system location. Then logic is added to determine if an achievement asset, either an icon or an evaluator, has a course override, or the system default should be used. In order to better validate path names, icons, evaluators, and notification templates are no longer allowed to have a slash in their name and be an absolute or relative path. In addition all paths are checked they are actual files and not directories. All course achievement assets must be placed in their appropriate directory without any subdirectories. The achievement list editor will add a message if the name is invalid (contains a slash) or is overriding a system asset, and now has javascript validation to not allow saving new data unless the evaluator file and icon file do not contain a slash. The achievement evaluator editor has been updated to inform the user if they are editing a system achievement and will create a course override, and provide a tab to revert to the system achievement. In addition JavaScript validation has been added to the achievement evaluator editor and achievement notification editor. Some icons were updated in this change to better work with dark mode. This includes adding a default achievement which is white circle with a black outline and a "?" in it to match other badges, a white background inside two icons in which this was not done, and there was a small white boundary around some of the level achievements that was removed. This patch was designed and mostly written by me, but Claude helped test it and provided some feedback, along with Claude mostly wrote the achievement notifications editor code based on what I already did to the achievement editor and help write the javascript validation for the slash in filenames on the AchivementList editor. One thing Claude pointed out was I didn't consider if an asset name was "." or "..", which would not lead to accessing the directory so wasn't technically needed. This lead me to change the tests from '-e' to '-f' to ensure a file exists that isn't a directory.
somiaj
force-pushed
the
move-achievements-out-of-course
branch
from
September 22, 2026 14:35
31f20e5 to
11c2115
Compare
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
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.
System achievements should not be placed in a course. This way any changes to them will propagate to all courses using them. Instead courses should be able to create local overrides of system achievements as needed.
This moves all of the default achievement evaluators and icons out of the modelCourse and places them in a system location. Then logic is added to determine if an achievement asset, either an icon or an evaluator, has a course override, or the system default should be used.
In order to better validate path names, icons, evaluators, and notification templates are no longer allowed to have a slash in their name and be an absolute or relative path. In addition all paths are checked they are actual files and not directories. All course achievement assets must be placed in their appropriate directory without any subdirectories.
The achievement list editor will add a message if the name is invalid (contains a slash) or is overriding a system asset, and now has javascript validation to not allow saving new data unless the evaluator file and icon file do not contain a slash. The achievement evaluator editor has been updated to inform the user if they are editing a system achievement and will create a course override, and provide a tab to revert to the system achievement.
In addition JavaScript validation has been added to the achievement evaluator editor and achievement notification editor.
Some icons were updated in this change to better work with dark mode. This includes adding a default achievement which is white circle with a black outline and a "?" in it to match other badges, a white background inside two icons in which this was not done, and there was a small white boundary around some of the level achievements that was removed.
This patch was designed and mostly written by me, but Claude helped test it and provided some feedback, along with Claude mostly wrote the achievement notifications editor code based on what I already did to the achievement editor and help write the javascript validation for the slash in filenames on the AchivementList editor. One thing Claude pointed out was I didn't consider if an asset name was "." or "..", which would not lead to accessing the directory so wasn't technically needed. This lead me to change the tests from '-e' to '-f' to ensure a file exists that isn't a directory.
This is a breaking change, and will require a bit of manual work on instructors to delete any overrides they don't want. I am unsure if an upgrade course path should be added. I suspect deleting overridden icons is safe, but it won't be safe for achievement evaluators, specifically level achievement evaluators I suspect are overridden a lot. The AchievementList when editing all achievements can inform a user what is being overridden, but they need to manually delete icons. The editor can be used to revert evaluators.
I don't know how bad the restricting file names to not contain a slash is going to be. My guess is it would be rare to manage achievements in their own sub-directories. Maybe @Alex-Jordan or @drdrew42 can comment on this, as they have access to lots of courses. That can be changed, but I don't think it is worth allowing sub-directories (but my use case maybe limited).