Use UUIDs for tests#189
Conversation
Signed-off-by: Raunak Madan <madanraunak24@gmail.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a modular approach to test metadata handling by creating several new partials for questions, options, and parent pages, alongside a new validation system for test metadata. It also includes a utility for generating stable UUIDs. The review feedback identifies a significant breaking change where dictionary keys were renamed from snake_case to camelCase, which may impact downstream consumers. Other feedback points to potential issues with the stability of generated keys, a type mismatch for the timeLimit field, and opportunities to refine the validation logic to prevent redundant error messages for missing IDs.
| "maxAttempts" $maxAttempts | ||
| "totalQuestionSets" $numberOfQuestionSets | ||
| "totalQuestionsInBank" (len $p.questions) | ||
| "passPercentage" (or $p.pass_percentage 70) | ||
| "timeLimit" (or $p.time_limit 0) | ||
| "questions" $questions | ||
| "totalQuestions" $totalQuestionsPerSet | ||
| "prerequisites" $normalizedPrerequisites | ||
| "totalMarks" $total | ||
| "parent" $parent | ||
| "next_page" $nextPage | ||
| "nextPage" $nextPage |
There was a problem hiding this comment.
The keys in the returned dictionary have been renamed from snake_case to camelCase (e.g., max_attempts to maxAttempts, total_question_sets to totalQuestionSets). This is a breaking change for any templates or JavaScript code that consumes the output of this partial. If this change is intentional, it should be documented, and all consumers should be updated. Otherwise, consider maintaining the existing naming convention for backward compatibility.
| {{- $orgId := $ctx.uuid -}} | ||
| {{- $id := partial "id.html" . -}} | ||
| {{- $quizID := partial "uuid.html" $id -}} | ||
| {{- $pageKey := printf "%s:%s" .File.Path .RelPermalink -}} |
There was a problem hiding this comment.
Using .File.Path and .RelPermalink to construct the pageKey makes the generated UUIDs for questions and options sensitive to file renaming or URL changes. Since $quizID (line 6) is already a stable identifier (derived from .Params.id or a hash of the permalink), it would be more robust to use $quizID as the base for question and option keys.
| {{- $pageKey := printf "%s:%s" .File.Path .RelPermalink -}} | |
| {{- $pageKey := $quizID -}} |
| "totalQuestionSets" $numberOfQuestionSets | ||
| "totalQuestionsInBank" (len $p.questions) | ||
| "passPercentage" (or $p.pass_percentage 70) | ||
| "timeLimit" (or $p.time_limit 0) |
There was a problem hiding this comment.
The timeLimit value is no longer being converted to a string. The previous implementation used (or $p.time_limit 0 | string). If the consumer of this partial expects a string value, this change could cause type mismatch issues.
| "timeLimit" (or $p.time_limit 0) | |
| "timeLimit" (or $p.time_limit 0 | string) |
| {{- if eq (strings.TrimSpace $questionID) "" -}} | ||
| {{- $message := printf "Question id is required [ %s ]" $page.File.Path -}} | ||
| {{- if eq $validationMode "error" -}} | ||
| {{- errorf "%s" $message -}} | ||
| {{- else if ne $validationMode "off" -}} | ||
| {{- warnf "%s" $message -}} | ||
| {{- end -}} | ||
| {{- end -}} | ||
| {{- if in $questionIds $questionID -}} | ||
| {{- $message := printf "Duplicate question id found: %s [ %s ]" $questionID $page.File.Path -}} | ||
| {{- if eq $validationMode "error" -}} | ||
| {{- errorf "%s" $message -}} | ||
| {{- else if ne $validationMode "off" -}} | ||
| {{- warnf "%s" $message -}} | ||
| {{- end -}} | ||
| {{- end -}} | ||
| {{- $questionIds = $questionIds | append $questionID -}} |
There was a problem hiding this comment.
The duplicate ID check will trigger for empty IDs if multiple questions are missing the id field. Since a warning/error for missing IDs is already handled on line 11, it's better to skip the duplicate check for empty strings to avoid redundant messages.
| {{- if eq (strings.TrimSpace $questionID) "" -}} | |
| {{- $message := printf "Question id is required [ %s ]" $page.File.Path -}} | |
| {{- if eq $validationMode "error" -}} | |
| {{- errorf "%s" $message -}} | |
| {{- else if ne $validationMode "off" -}} | |
| {{- warnf "%s" $message -}} | |
| {{- end -}} | |
| {{- end -}} | |
| {{- if in $questionIds $questionID -}} | |
| {{- $message := printf "Duplicate question id found: %s [ %s ]" $questionID $page.File.Path -}} | |
| {{- if eq $validationMode "error" -}} | |
| {{- errorf "%s" $message -}} | |
| {{- else if ne $validationMode "off" -}} | |
| {{- warnf "%s" $message -}} | |
| {{- end -}} | |
| {{- end -}} | |
| {{- $questionIds = $questionIds | append $questionID -}} | |
| {{- if eq (strings.TrimSpace $questionID) "" -}} | |
| {{- $message := printf "Question id is required [ %s ]" $page.File.Path -}} | |
| {{- if eq $validationMode "error" -}} | |
| {{- errorf "%s" $message -}} | |
| {{- else if ne $validationMode "off" -}} | |
| {{- warnf "%s" $message -}} | |
| {{- end -}} | |
| {{- else -}} | |
| {{- if in $questionIds $questionID -}} | |
| {{- $message := printf "Duplicate question id found: %s [ %s ]" $questionID $page.File.Path -}} | |
| {{- if eq $validationMode "error" -}} | |
| {{- errorf "%s" $message -}} | |
| {{- else if ne $validationMode "off" -}} | |
| {{- warnf "%s" $message -}} | |
| {{- end -}} | |
| {{- end -}} | |
| {{- $questionIds = $questionIds | append $questionID -}} | |
| {{- end -}} |
| {{- if eq (strings.TrimSpace $optionID) "" -}} | ||
| {{- $message := printf "Option id is required for question id: %s [ %s ]" $questionID $page.File.Path -}} | ||
| {{- if eq $validationMode "error" -}} | ||
| {{- errorf "%s" $message -}} | ||
| {{- else if ne $validationMode "off" -}} | ||
| {{- warnf "%s" $message -}} | ||
| {{- end -}} | ||
| {{- end -}} | ||
| {{- if in $optionIds $optionID -}} | ||
| {{- $message := printf "Duplicate option id found: %s for question id: %s [ %s ]" $optionID $questionID $page.File.Path -}} | ||
| {{- if eq $validationMode "error" -}} | ||
| {{- errorf "%s" $message -}} | ||
| {{- else if ne $validationMode "off" -}} | ||
| {{- warnf "%s" $message -}} | ||
| {{- end -}} | ||
| {{- end -}} | ||
| {{- $optionIds = $optionIds | append $optionID -}} |
There was a problem hiding this comment.
Similar to the question ID check, the duplicate check for option IDs should be skipped if the ID is missing to avoid redundant error messages.
| {{- if eq (strings.TrimSpace $optionID) "" -}} | |
| {{- $message := printf "Option id is required for question id: %s [ %s ]" $questionID $page.File.Path -}} | |
| {{- if eq $validationMode "error" -}} | |
| {{- errorf "%s" $message -}} | |
| {{- else if ne $validationMode "off" -}} | |
| {{- warnf "%s" $message -}} | |
| {{- end -}} | |
| {{- end -}} | |
| {{- if in $optionIds $optionID -}} | |
| {{- $message := printf "Duplicate option id found: %s for question id: %s [ %s ]" $optionID $questionID $page.File.Path -}} | |
| {{- if eq $validationMode "error" -}} | |
| {{- errorf "%s" $message -}} | |
| {{- else if ne $validationMode "off" -}} | |
| {{- warnf "%s" $message -}} | |
| {{- end -}} | |
| {{- end -}} | |
| {{- $optionIds = $optionIds | append $optionID -}} | |
| {{- if eq (strings.TrimSpace $optionID) "" -}} | |
| {{- $message := printf "Option id is required for question id: %s [ %s ]" $questionID $page.File.Path -}} | |
| {{- if eq $validationMode "error" -}} | |
| {{- errorf "%s" $message -}} | |
| {{- else if ne $validationMode "off" -}} | |
| {{- warnf "%s" $message -}} | |
| {{- end -}} | |
| {{- else -}} | |
| {{- if in $optionIds $optionID -}} | |
| {{- $message := printf "Duplicate option id found: %s for question id: %s [ %s ]" $optionID $questionID $page.File.Path -}} | |
| {{- if eq $validationMode "error" -}} | |
| {{- errorf "%s" $message -}} | |
| {{- else if ne $validationMode "off" -}} | |
| {{- warnf "%s" $message -}} | |
| {{- end -}} | |
| {{- end -}} | |
| {{- $optionIds = $optionIds | append $optionID -}} | |
| {{- end -}} |
1 participant
Notes for Reviewers
This PR fixes #
Signed commits