Fix 342 Pair Programming Bug Fixes - #345
Conversation
there may be old users and new users in the same import. If using roleMap as a guard, the roles of old users wouldn't be updated.
| const { assertStableEmailTemplateContent } = require("../../utils/helper/templateResolver"); | ||
|
|
||
| const MAIL_SERVICE_KEY_PREFIX = "system.mailService."; | ||
| const PRESERVE_WHITESPACE_SETTING_TYPES = new Set(["edits", "text"]); |
There was a problem hiding this comment.
PRESERVE_WHITESPACE_SETTING_TYPES lists the types to skip, so shouldTrimSetting trims every type that is not in the set. Any setting type added later, and any entry whose type could not be resolved, gets trimmed by default without anyone noticing. Flip it to an allow-list of types that should be trimmed (for example "string", "number", "select", "color"), so a new type has to opt in.
| * @returns {Promise<Map<string, string>>} | ||
| */ | ||
| async function getSettingTypeByKey(Setting, settings, options = {}) { | ||
| if (typeof Setting.findAll !== "function") { |
There was a problem hiding this comment.
Both callers of saveSettings pass a real Sequelize model, so Setting.findAll is always a function and this branch never runs. If it ever did run it would return an empty Map, and every setting would fall back to being trimmed, including the "text" and "edits" types this PR wants to protect. Drop the guard, or throw here, rather than silently changing how values are saved.
cdcacf2 to
d65cdc5
Compare
| */ | ||
| async bulkCreateUsers(data) { | ||
| const users = data["users"]; | ||
| const roleMap = data["roleMap"]; |
There was a problem hiding this comment.
roleMap comes from the client, and assignUserRoles in backend/db/models/user.js assigns any role name it gets. A caller can send roleMap: {"Student*in": "admin"} together with an existing user's email and make that account an admin, because userBulkCreate has no isAdmin() check (the other handlers in this class do have one). Please guard the handler with if (!(await this.isAdmin())) throw ... and drop any mapped value that is not in the allowed role list, since the "no admin" filter in RoleMappingStep.vue only applies to the dropdown.
| </template> | ||
| <!-- Step1: Upload --> | ||
| <template #step-1> | ||
| <template #step="{ step }"> |
There was a problem hiding this comment.
All five steps now render from one slot in this file, which the lint job already flags at 642 lines (the limit is 300). RoleMappingStep.vue in this PR shows the shape that works here. Please move the preview, confirm and result blocks into their own step components as well, and keep this template down to picking which one to show.
| computed: { | ||
| careRoleOptions() { | ||
| return [ | ||
| ...this.systemRoles.filter((role) => !role.deleted && role.name !== "admin").map((role) => ({ |
There was a problem hiding this comment.
role.deleted is never set on these objects. sendSystemRoles in backend/webserver/sockets/app.js sends only id and name, so this check removes nothing and soft-deleted roles still appear in the dropdown. Add where: {deleted: false} to that findAll and drop the !role.deleted part here.
| value: role.name, | ||
| label: role.name.charAt(0).toUpperCase() + role.name.slice(1), | ||
| })), | ||
| { value: "", label: "Do not assign additional role" } |
There was a problem hiding this comment.
This option sounds like the user keeps the roles they already have, but that is not what happens. When an existing user is imported again, assignUserRoles in backend/db/models/user.js first deletes all their roles and then adds back only user plus the mapped ones. Please rename it to something like "No extra CARE role" and add a note on this step that old roles are removed.
|
|
||
| Moodle role labels can vary by Moodle instance and may include multilingual markup such as ``{mlang de}Lehrende{mlang}{mlang other}Lecturer{mlang}``. CARE hides this markup in the mapping step while keeping the original Moodle role string for the import. | ||
|
|
||
| CSV user import also includes a role mapping step. CARE reads the distinct values from the CSV ``roles`` column and asks you to map each value to a CARE role before previewing the import. Multiple roles in one CSV cell should be separated by commas. |
There was a problem hiding this comment.
CARE docs name the role instead of addressing the reader, so "asks you to map" does not match the rest of this page. Please write "asks the admin to map each value to a CARE role".
2 participants
Main Description
This pull request contains fix to issues found during the last pair programming session, which list as follows: