Introduce editable table for creating score distributions #5723
Conversation
WalkthroughThis update introduces modifications to the database schema and the package dependencies. A new column named Changes
Assessment against linked issues
Suggested labels
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Outside diff range, codebase verification and nitpick comments (1)
app/assets/javascripts/components/input_table.ts (1)
91-121: Remove commented-out condition infirstUpdated.The
firstUpdatedmethod is well-structured, but consider removing the commented-out condition in the resize observer for clarity.- // if (Math.abs(parseInt(this.table.getWidth(2) as string) - this.descriptionColWidth) > 10) {Tools
GitHub Check: codecov/patch
[warning] 91-93: app/assets/javascripts/components/input_table.ts#L91-L93
Added lines #L91 - L93 were not covered by tests
[warning] 116-116: app/assets/javascripts/components/input_table.ts#L116
Added line #L116 was not covered by tests
[warning] 118-118: app/assets/javascripts/components/input_table.ts#L118
Added line #L118 was not covered by tests
There was a problem hiding this comment.
Row removal is a lot clearer now 👍
A few more comments:
- the row remove button has the material mouseover circle, but this is a bit strange in the table which is style differently (and part of the circle is cut off)
- the visibility checkbox in the table is aligned badly (bottom-left)
- I apparently forgot to write down my comment on the max score visibility. The toggle there looks very strange
- I'm not totally convinced that swapping out the pencil for "edit" fixed the problem. To me, there's a disconnect between the button and the table however I have no clear suggestion on how to fix this.
- not changed in this PR but strange when I think about it: the bottom row now says maximum score but should this be total score?
Sorry for nitpicking!
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
app/assets/javascripts/components/input_table.ts (2)
293-293: Review the necessity ofcontenteditable="true"on the table containerThe
<div>containing the table reference (this.tableRef) has the attributecontenteditable="true". Sincejspreadsheetmanages cell editing internally, settingcontenteditableon the container may not be necessary and could introduce unintended behavior or security issues. Consider removing this attribute if it's not required for the functionality.
246-252: Improve warning message display inconfirmWarnings()The warnings concatenated with
\nmay not render newlines properly in theconfirmdialog across all browsers, potentially leading to a less clear message for the user. To enhance user experience, consider using a custom modal dialog that properly formats multiline messages or employs line breaks that are consistently rendered.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
app/assets/javascripts/components/input_table.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
app/assets/javascripts/components/input_table.ts
[error] 277-277: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().
(lint/security/noGlobalEval)
|
On the delete button => I made a new simpler button type where hovering is indicated by opacity reduction For the edit button => I removed the edit button completely in this version. Instead I added some color indication to table cells when hovering them, making it more clear that they are editable. Peek.2024-12-03.15-56.mp4 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (10)
test/controllers/evaluation_exercise_controller_test.rb (3)
22-38: Consider adding test coverage for edge cases.While the test covers the basic visibility toggle scenarios, consider adding tests for:
- Concurrent visibility updates
- Race conditions when multiple users update visibility simultaneously
test 'can update visibility as course admin' do + # Test concurrent updates + threads = [] + 5.times do + threads << Thread.new do + @exercise.update!(visible_score: true) + patch evaluation_exercise_path(@exercise, format: :js), params: { + evaluation_exercise: { visible_score: false } + } + end + end + threads.each(&:join) + @exercise.reload + assert_not @exercise.visible_score?
44-88: Enhance test coverage for score item updates.The test verifies basic CRUD operations but could benefit from additional scenarios:
- Boundary testing for maximum values
- Special characters in descriptions
- Very large score item lists
test 'should update all score items if course administrator' do + # Test boundary values + patch evaluation_exercise_path(@exercise, format: :json), params: { + evaluation_exercise: { + score_items: [ + { name: 'max', description: 'Maximum value test', maximum: '999999.99' }, + { name: 'special', description: 'Special chars: !@#$%^&*()', maximum: '1.0' } + ] + } + } + assert_response :success
129-158: Consider testing bulk delete operations.While the test covers basic deletion, consider adding tests for:
- Bulk deletion of multiple score items
- Deletion of score items with associated data
test 'should delete score item if course administrator' do + # Test bulk deletion + score_items = create_list :score_item, 10, evaluation_exercise: @exercise + assert_equal 10, @exercise.score_items.count + + patch evaluation_exercise_path(@exercise, format: :json), params: { + evaluation_exercise: { score_items: [] } + } + + assert_response :success + assert_equal 0, @exercise.score_items.countconfig/locales/js/en.yml (1)
312-317: Use proper ellipsis character in spreadsheet actionsFor consistency and proper typography, use the ellipsis character (…) instead of three dots.
- copy: Copy... + copy: Copy… - paste: Paste... + paste: Paste…app/assets/javascripts/components/input_table.ts (1)
242-244: Improve maximum score validationThe current regex validation could be more precise and the error message could be more helpful.
- if (!/^\d{1,3}(\.\d+)?$/.test(item.maximum) || parseFloat(item.maximum) <= 0) { + const max = parseFloat(item.maximum); + if (!/^\d{1,3}(\.\d{1,2})?$/.test(item.maximum) || Number.isNaN(max) || max <= 0 || max > 1000) { invalidCells.push("D" + row); + console.warn(`Invalid maximum score: ${item.maximum}. Must be a number between 0 and 1000 with up to 2 decimal places.`); }app/assets/javascripts/i18n/translations.json (2)
351-351: Improve clarity of total_visible messageThe current message could be clearer about what "total score" means.
Consider this improvement:
- "total_visible": "Make the total score visible to students once the evaluation is released.", + "total_visible": "Make the sum of all score items visible to students once the evaluation is released.",
869-869: Improve Dutch translation of total_visibleFor consistency with the suggested English improvement, update the Dutch translation as well.
Consider this improvement:
- "total_visible": "Maak totaal score zichtbaar voor studenten eens de evaluatie vrijgegeven is.", + "total_visible": "Maak de som van alle scoreonderdelen zichtbaar voor studenten eens de evaluatie vrijgegeven is.",app/controllers/evaluation_exercise_controller.rb (3)
10-11: Ensure compatibility offiltermethod usageThe
filtermethod is an alias forselectintroduced in Ruby 2.6. If the application needs to support Ruby versions earlier than 2.6, consider usingselectinstead for broader compatibility.
20-20: Encapsulate Parameters When Creating NewScoreItemFor consistency and maintainability, use the
score_item_paramsmethod when creating newScoreIteminstances.Update the creation call:
- @evaluation_exercise.score_items.create!(item.permit(:name, :description, :maximum, :visible, :order)) + @evaluation_exercise.score_items.create!(score_item_params(item))
29-30: Avoid Redundant Reloads of@evaluation_exercise
@evaluation_exerciseis reloaded twice—once implicitly through@evaluation.reloadand again with@evaluation_exercise.reload. Ensure both reloads are necessary to prevent unnecessary database queries.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (20)
app/assets/javascripts/components/input_table.ts(1 hunks)app/assets/javascripts/i18n/translations.json(2 hunks)app/assets/javascripts/score_item.ts(2 hunks)app/assets/stylesheets/components/btn.css.scss(1 hunks)app/assets/stylesheets/models/score_items.css.scss(2 hunks)app/controllers/evaluation_exercise_controller.rb(1 hunks)app/controllers/evaluations_controller.rb(1 hunks)app/controllers/score_items_controller.rb(0 hunks)app/policies/evaluation_policy.rb(0 hunks)app/policies/score_item_policy.rb(0 hunks)app/views/score_items/_exercise.html.erb(3 hunks)app/views/score_items/_score_item.json.jbuilder(0 hunks)app/views/score_items/show.json.jbuilder(0 hunks)config/locales/js/en.yml(1 hunks)config/locales/js/nl.yml(1 hunks)config/locales/views/score_items/en.yml(1 hunks)config/locales/views/score_items/nl.yml(1 hunks)config/routes.rb(0 hunks)test/controllers/evaluation_exercise_controller_test.rb(2 hunks)test/controllers/score_items_controller_test.rb(0 hunks)
💤 Files with no reviewable changes (7)
- app/views/score_items/show.json.jbuilder
- app/views/score_items/_score_item.json.jbuilder
- app/policies/evaluation_policy.rb
- app/policies/score_item_policy.rb
- config/routes.rb
- app/controllers/score_items_controller.rb
- test/controllers/score_items_controller_test.rb
🚧 Files skipped from review as they are similar to previous changes (2)
- app/assets/stylesheets/models/score_items.css.scss
- config/locales/views/score_items/en.yml
🧰 Additional context used
🪛 Biome (1.9.4)
app/assets/javascripts/components/input_table.ts
[error] 294-294: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().
(lint/security/noGlobalEval)
🪛 GitHub Check: codecov/patch
app/assets/javascripts/components/input_table.ts
[warning] 1-9: app/assets/javascripts/components/input_table.ts#L1-L9
Added lines #L1 - L9 were not covered by tests
[warning] 23-23: app/assets/javascripts/components/input_table.ts#L23
Added line #L23 was not covered by tests
[warning] 38-38: app/assets/javascripts/components/input_table.ts#L38
Added line #L38 was not covered by tests
[warning] 40-40: app/assets/javascripts/components/input_table.ts#L40
Added line #L40 was not covered by tests
[warning] 42-42: app/assets/javascripts/components/input_table.ts#L42
Added line #L42 was not covered by tests
[warning] 44-44: app/assets/javascripts/components/input_table.ts#L44
Added line #L44 was not covered by tests
[warning] 46-46: app/assets/javascripts/components/input_table.ts#L46
Added line #L46 was not covered by tests
[warning] 50-50: app/assets/javascripts/components/input_table.ts#L50
Added line #L50 was not covered by tests
[warning] 52-52: app/assets/javascripts/components/input_table.ts#L52
Added line #L52 was not covered by tests
[warning] 54-56: app/assets/javascripts/components/input_table.ts#L54-L56
Added lines #L54 - L56 were not covered by tests
[warning] 60-61: app/assets/javascripts/components/input_table.ts#L60-L61
Added lines #L60 - L61 were not covered by tests
[warning] 64-65: app/assets/javascripts/components/input_table.ts#L64-L65
Added lines #L64 - L65 were not covered by tests
[warning] 68-68: app/assets/javascripts/components/input_table.ts#L68
Added line #L68 was not covered by tests
[warning] 70-70: app/assets/javascripts/components/input_table.ts#L70
Added line #L70 was not covered by tests
[warning] 74-75: app/assets/javascripts/components/input_table.ts#L74-L75
Added lines #L74 - L75 were not covered by tests
[warning] 78-80: app/assets/javascripts/components/input_table.ts#L78-L80
Added lines #L78 - L80 were not covered by tests
[warning] 91-92: app/assets/javascripts/components/input_table.ts#L91-L92
Added lines #L91 - L92 were not covered by tests
[warning] 94-95: app/assets/javascripts/components/input_table.ts#L94-L95
Added lines #L94 - L95 were not covered by tests
[warning] 109-111: app/assets/javascripts/components/input_table.ts#L109-L111
Added lines #L109 - L111 were not covered by tests
[warning] 114-115: app/assets/javascripts/components/input_table.ts#L114-L115
Added lines #L114 - L115 were not covered by tests
[warning] 118-118: app/assets/javascripts/components/input_table.ts#L118
Added line #L118 was not covered by tests
[warning] 121-122: app/assets/javascripts/components/input_table.ts#L121-L122
Added lines #L121 - L122 were not covered by tests
[warning] 125-127: app/assets/javascripts/components/input_table.ts#L125-L127
Added lines #L125 - L127 were not covered by tests
[warning] 129-132: app/assets/javascripts/components/input_table.ts#L129-L132
Added lines #L129 - L132 were not covered by tests
[warning] 136-136: app/assets/javascripts/components/input_table.ts#L136
Added line #L136 was not covered by tests
[warning] 138-140: app/assets/javascripts/components/input_table.ts#L138-L140
Added lines #L138 - L140 were not covered by tests
[warning] 142-144: app/assets/javascripts/components/input_table.ts#L142-L144
Added lines #L142 - L144 were not covered by tests
[warning] 146-147: app/assets/javascripts/components/input_table.ts#L146-L147
Added lines #L146 - L147 were not covered by tests
[warning] 149-149: app/assets/javascripts/components/input_table.ts#L149
Added line #L149 was not covered by tests
[warning] 151-151: app/assets/javascripts/components/input_table.ts#L151
Added line #L151 was not covered by tests
🔇 Additional comments (20)
test/controllers/evaluation_exercise_controller_test.rb (2)
8-8: LGTM! Good practice using instance variables in setup.
The setup method correctly initializes @exercise for reuse across test cases, promoting DRY principles.
90-127: LGTM! Comprehensive validation testing.
The test cases effectively cover invalid data scenarios, including missing data and negative values.
app/assets/stylesheets/components/btn.css.scss (1)
211-222: LGTM! Clean implementation of inline icon button styles.
The new .btn-icon-inline class appropriately handles flexible sizing and hover states. The implementation follows the existing pattern of the codebase.
app/controllers/evaluations_controller.rb (1)
130-130: Verify the user experience impact of the redirect change.
The fallback location has been changed to edit_evaluation_path(@evaluation). This could affect the user's workflow when the browser's back functionality fails.
✅ Verification successful
Let me gather more context about the redirects and user workflow in this controller.
Let me check the context of where this redirect is being used to fully understand the workflow.
The redirect change in modify_grading_visibility action is appropriate and safe
The redirect_back with edit_evaluation_path(@evaluation) fallback is used in the modify_grading_visibility action, which is a visibility toggle operation. The fallback to the evaluation edit page is a logical choice because:
- It's used after a state change operation (
change_grade_visibility!) - The edit page is where users manage evaluation settings
- The controller shows consistent redirect patterns, using evaluation paths for similar operations
- Test coverage confirms the expected redirect behavior
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential redirect chains or loops
# Test: Search for redirects to edit_evaluation_path
rg -A 5 "redirect.*edit_evaluation_path"
# Test: Search for other references to the old path
rg "evaluation_score_items_path"
Length of output: 1422
Script:
#!/bin/bash
# Check the full context of the evaluations controller to understand the workflow
rg -B 5 -A 5 "def destroy" app/controllers/evaluations_controller.rb
# Look for any other redirects in the same controller
rg "redirect_" app/controllers/evaluations_controller.rb
# Check the routes definition for evaluation paths
rg "resources :evaluation" config/routes.rb
Length of output: 1218
Script:
#!/bin/bash
# Get more context around the redirect_back line
rg -B 10 -A 5 "redirect_back.*edit_evaluation_path" app/controllers/evaluations_controller.rb
# Check for the action/method containing this redirect
ast-grep --pattern 'def $_ {
$$$
redirect_back fallback_location: edit_evaluation_path($_)
$$$
}'
Length of output: 683
config/locales/js/en.yml (2)
304-304: 🛠️ Refactor suggestion
Update maximum score help text to avoid confusion
Based on the PR comments, while the UI buttons use 0.25 increments, the system actually allows steps of 0.01. The current help text might mislead users.
- maximum_help: The maximum grade for this score item. The grade should be between 0 and 1000, and works in increments of 0.25.
+ maximum_help: The maximum grade for this score item. The grade should be between 0 and 1000. You can use the buttons to adjust in steps of 0.25, or type any value with up to 2 decimal places.Likely invalid or redundant comment.
305-307: 🛠️ Refactor suggestion
Add warning message for reordering score items
The PR objectives mention that reordering score items after some solutions have been evaluated may require reevaluation. This important information should be communicated to users.
modified_warning: You have changed the maximum score of one or more score items. This will mark all completed evaluations with this score item as uncompleted.
deleted_warning: You have deleted one or more score items. This will also delete all scores for these items.
validation_warning: All score items must have a name and a maximum score, and the maximum score must be between 0 and 1000.
+ reorder_warning: Reordering score items after some solutions have been evaluated may require those solutions to be reevaluated.Likely invalid or redundant comment.
app/assets/javascripts/components/input_table.ts (2)
293-295:
Replace eval() with safer response handling
Using eval() on the response text is a security risk as it can execute arbitrary JavaScript code.
if (response.ok) {
- const js = await response.text();
- eval(js);
+ const data = await response.json();
+ // Handle the response data appropriately
+ this.dispatchEvent(new CustomEvent('save-success', { detail: data }));
}Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 294-294: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().
(lint/security/noGlobalEval)
217-219:
Prevent memory leaks from ResizeObserver
The ResizeObserver is not properly cleaned up when the component is removed, which could lead to memory leaks.
+ private resizeObserver: ResizeObserver;
+
protected firstUpdated(_changedProperties: PropertyValues): void {
super.firstUpdated(_changedProperties);
this.initTable();
- new ResizeObserver(() => {
+ this.resizeObserver = new ResizeObserver(() => {
this.table.setWidth(2, this.descriptionColWidth);
}).observe(this.tableRef.value);
}
+
+ disconnectedCallback(): void {
+ super.disconnectedCallback();
+ if (this.resizeObserver) {
+ this.resizeObserver.disconnect();
+ }
+ if (this.table) {
+ this.table.destroy();
+ }
+ }Likely invalid or redundant comment.
config/locales/js/nl.yml (3)
312-317: Use proper ellipsis character in spreadsheet actions
For consistency and proper typography, use the ellipsis character (…) instead of three dots.
- copy: Kopieer...
+ copy: Kopieer…
- paste: Plak...
+ paste: Plak…304-304: 🛠️ Refactor suggestion
Update maximum score help text to avoid confusion
The help text should be updated to match the actual behavior of the system.
- maximum_help: De maximumscore voor dit scoreonderdeel. Dit moet een getal zijn tussen 0 en 1000 en gaat in stappen van 0.25.
+ maximum_help: De maximumscore voor dit scoreonderdeel. Dit moet een getal zijn tussen 0 en 1000. Je kan de knoppen gebruiken om aan te passen in stappen van 0.25, of een waarde typen met maximaal 2 decimalen.307-309: 🛠️ Refactor suggestion
Add warning message for reordering score items
Add a Dutch translation for the reordering warning message.
modified_warning: "Je hebt de maximumscore van een of meerdere scoreonderdelen aangepast. Alle afgewerkte evaluaties met dit scoreonderdeel zullen terug als onafgewerkt gemarkeerd worden."
deleted_warning: "Je hebt een of meerdere scoreonderdelen verwijderd. Dit zal ook de bijhorende scores van de studenten verwijderen."
validation_warning: "Alle scoreonderdelen moeten een naam en een maximumscore hebben. De maximumscore moet een getal zijn tussen 0 en 1000."
+ reorder_warning: "Het herordenen van scoreonderdelen nadat sommige oplossingen al geëvalueerd zijn, kan vereisen dat deze oplossingen opnieuw geëvalueerd worden."app/assets/javascripts/i18n/translations.json (4)
347-347: Update maximum_help text to match actual behavior
According to the PR comments, while the UI buttons use 0.25 steps, the system allows steps of 0.01.
Apply this change to clarify the behavior:
- "maximum_help": "The maximum grade for this score item. The grade should be between 0 and 1000, and works in increments of 0.25.",
+ "maximum_help": "The maximum grade for this score item. The grade should be between 0 and 1000. You can enter any value with up to 2 decimal places, or use the up/down buttons for 0.25 increments.",865-865: Update Dutch maximum_help text to match English changes
The Dutch translation should be updated to match the clarified English text about decimal steps.
Apply this change:
- "maximum_help": "De maximumscore voor dit scoreonderdeel. Dit moet een getal zijn tussen 0 en 1000 en gaat in stappen van 0.25.",
+ "maximum_help": "De maximumscore voor dit scoreonderdeel. Dit moet een getal zijn tussen 0 en 1000. Je kan een getal ingeven met maximaal 2 decimalen, of de pijltjes gebruiken voor stappen van 0.25.",333-355:
Add reordering warning message
The PR objectives mention that reordering score items after solutions are evaluated requires reevaluation, but there's no warning message for this.
Add this translation:
"score_items": {
+ "reorder_warning": "Reordering score items after solutions have been evaluated will require reevaluation of those solutions.",
...
}Likely invalid or redundant comment.
851-873:
Add Dutch translation for reorder warning message
The Dutch translation for the reordering warning message needs to be added to match the English version.
Add this translation:
"score_items": {
+ "reorder_warning": "Het herordenen van scoreonderdelen nadat oplossingen zijn geëvalueerd, vereist een herevaluatie van die oplossingen.",
...
}Likely invalid or redundant comment.
config/locales/views/score_items/nl.yml (1)
39-39: Remove Duplicate Translation Key
The translation key edit: "Bewerken" is defined twice, both under the table section and at line 39. Duplicate keys can cause unexpected behavior and should be removed.
Apply this diff to remove the duplicate:
- edit: Bewerkenapp/assets/javascripts/score_item.ts (2)
53-55: Add Null Checks for DOM Elements
The code assumes that table and form elements exist. To prevent potential runtime errors, add null checks to ensure these elements are present before adding event listeners.
Apply this improvement:
export function initEditButton(element: HTMLElement): void {
- const table = element.querySelector(".score-items-table") as HTMLTableElement;
- const form = element.querySelector("d-score-item-input-table") as ScoreItemInputTable;
+ const table = element.querySelector<HTMLTableElement>(".score-items-table");
+ const form = element.querySelector<ScoreItemInputTable>("d-score-item-input-table");
+ if (!table || !form) {
+ console.error("Required elements not found for score item editing.");
+ return;
+ }56-58: Improve Click Target Specificity and Accessibility
Attaching a click handler to the entire table may interfere with other interactive elements and affect accessibility. Consider adding a dedicated edit button and enhancing keyboard support.
Suggested refactor:
- table.addEventListener("click", () => {
+ const editBtn = element.querySelector(".edit-btn");
+ if (!editBtn) {
+ console.error("Edit button not found.");
+ return;
+ }
+ // Add role and tabindex for accessibility
+ editBtn.setAttribute("role", "button");
+ editBtn.setAttribute("tabindex", "0");
+ editBtn.addEventListener("click", () => {
table.classList.add("d-none");
form.classList.remove("d-none");
});
+ // Support keyboard interaction
+ editBtn.addEventListener("keydown", (e: KeyboardEvent) => {
+ if (e.key === "Enter" || e.key === " ") {
+ e.preventDefault();
+ table.classList.add("d-none");
+ form.classList.remove("d-none");
+ }
+ });app/views/score_items/_exercise.html.erb (2)
41-43: Use Correct Size Classes for Material Design Icons
The class mdi-18 should be mdi-18px to correctly set the icon size according to Material Design Icons guidelines.
Apply this diff to fix the icon size:
- <i class="mdi mdi-18 mdi-eye"></i>
+ <i class="mdi mdi-18px mdi-eye"></i>
- <i class="mdi mdi-18 mdi-eye-off"></i>
+ <i class="mdi mdi-18px mdi-eye-off"></i>Also applies to: 56-58
68-68: Properly Escape JSON Data in HTML Attributes
Embedding raw JSON in HTML attributes can lead to parsing issues due to unescaped characters. Use the j helper to safely escape the JSON data.
Apply this diff to escape the JSON:
- score-items="<%= evaluation_exercise.score_items.to_json %>"
+ score-items="<%= j evaluation_exercise.score_items.to_json %>"
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
app/views/score_items/_exercise.html.erb (2)
45-50: Consider improving button visibilityThe "Add" button might not be prominent enough with just the
btn-textclass. Consider using a more visible button style to align with the PR's goal of improving user interaction.- <a class="btn btn-text edit-btn"> + <a class="btn btn-primary edit-btn">
76-76: Add error handling for element initializationConsider adding error handling in case the element isn't found to prevent potential JavaScript errors.
- window.dodona.initEditButton(document.getElementById("score-items-<%= evaluation_exercise.id %>")); + const scoreItemsElement = document.getElementById("score-items-<%= evaluation_exercise.id %>"); + if (scoreItemsElement) { + window.dodona.initEditButton(scoreItemsElement); + } else { + console.warn(`Score items element not found for evaluation exercise ${<%= evaluation_exercise.id %>}`); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
app/assets/stylesheets/models/score_items.css.scss(4 hunks)app/views/score_items/_exercise.html.erb(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/assets/stylesheets/models/score_items.css.scss
🔇 Additional comments (6)
app/views/score_items/_exercise.html.erb (6)
6-8: LGTM: Clean header formatting
The header structure is clean and properly formatted.
10-12: Good accessibility improvement
Adding the title attribute enhances screen reader support for the table.
24-26: LGTM: Proper colspan and i18n usage
The placeholder text is correctly implemented with proper colspan and internationalization.
35-38: Use correct size classes for Material Design Icons
The icon size class should be mdi-18px instead of mdi-18.
55-58: Use correct size classes for Material Design Icons
The icon size class should be mdi-18px instead of mdi-18.
65-72:
Properly escape JSON data in HTML attributes
The JSON data in the score-items attribute needs proper escaping to prevent potential XSS vulnerabilities.
This pull request introduces jspreadsheet-ce as table editor to manage score items for an evaluation.
This way, copying, reusing and even initially filling it out all go a lot faster. The excel like layout should be easy to interact with for most users.
It also allows to prepare score items in advance in a spreadsheet program and pasting it here directly.
Reordering is partially supported. Doing when some solutions are already evaluated will force you to redo those evaluations.
Peek.2024-12-09.13-41.mp4
Darkmode is also supported
Closes #4940 and closes #2713