Conversation
31f20e5 to
11c2115
Compare
drgrice1
left a comment
There was a problem hiding this comment.
I am a little confused as to the reason for the changes to the allowed path names for the achievement notification templates. The achievement notifications are still in the course's templates/achievements/notifications directory, there is no system override, and it was already coded to handle more general paths. So why is the new restriction needed? Is it just to obtain JavaScript validation?
|
|
||
| # Convert long paths to [ACHEVNOTIFYDIR] | ||
| # Append [ACHEVNOTIFYDIR] to filename. | ||
| sub shortPath ($c, $file) { |
There was a problem hiding this comment.
This method needs to be renamed now. It does not do the same thing at all compared to what it used to do. It now appends a prefix to a file name, instead of shortening an input path.
There was a problem hiding this comment.
I'll update this depending on if we prefer to not go as strict as my original design of not allowing subdirectories or not.
|
My logic for adding the restriction to the achievement notifications was not for validation, but to be consistent with the other achievement assets. I can revert those changes, or update all changes to allow assets to live in subdirectories, depending on what we think is best. I think in general allowing such structure and extra path checking isn't needed, but my use case could be limited. |
11c2115 to
ab2e5c1
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.
ab2e5c1 to
798195a
Compare
|
I see. Well, I guess we will see how others feel about allowing sub-directories for these things. I just thought it was a bit odd to remove already implemented functionality. |
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).