Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Clarify Disable Autolab Submissions Option #2224

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Clarify Disable Autolab Submissions Option #2224

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
b8af670
Reformat UI of Edit Assessment page to make disabling assessments mor…
jhs-panda Sep 27, 2024
20e9ad2
fix formatting from diff
jhs-panda Sep 27, 2024
0ac3f1a
Clarify message on submission page.
jhs-panda Sep 27, 2024
a9a71e2
corrected errors
jhs-panda Oct 27, 2024
377b22f
Remove irrelevant files.
jhs-panda Oct 27, 2024
e022e01
Add pattern checking for the handin filename field (from CodeRabbit)
jhs-panda Nov 15, 2024
468f0e4
Changed variables from var to const (CodeRabbit suggestion)
jhs-panda Nov 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 29 additions & 7 deletions app/views/assessments/_edit_handin.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,22 @@
<% content_for :javascripts do %>
<%= javascript_include_tag "init_handin_datetimepickers" %>
<script>
function toggleHandinAutolab(disable_button) {
const handinField = document.querySelector('.handin-filename-field');
handinField.disabled = disable_button.checked;
if (!handinField.disabled && !handinField.value) {
handinField.value = 'handin.c';
}
}

document.addEventListener('DOMContentLoaded', function() {
const disableHandinsCheckbox = document.querySelector('.disable-handins-toggle');
toggleHandinAutolab(disableHandinsCheckbox);
disableHandinsCheckbox.addEventListener('change', function() {
toggleHandinAutolab(disableHandinsCheckbox);
});
});
</script>
Comment on lines +3 to +19
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add error handling and defensive programming.

The JavaScript implementation needs more robust error handling to prevent potential runtime errors.

Apply these improvements:

 <script>
+    const DEFAULT_FILENAME = 'handin.c';
+
     function toggleHandinAutolab(disable_button) {
+        if (!disable_button) {
+            console.error('Invalid disable button parameter');
+            return;
+        }
         const handinField = document.querySelector('.handin-filename-field');
+        if (!handinField) {
+            console.error('Handin filename field not found');
+            return;
+        }
         handinField.disabled = disable_button.checked;
         if (!handinField.disabled && !handinField.value) {
-            handinField.value = 'handin.c';
+            handinField.value = DEFAULT_FILENAME;
         }
     }

     document.addEventListener('DOMContentLoaded', function() {
         const disableHandinsCheckbox = document.querySelector('.disable-handins-toggle');
+        if (!disableHandinsCheckbox) {
+            console.error('Disable handins checkbox not found');
+            return;
+        }
         toggleHandinAutolab(disableHandinsCheckbox);
         disableHandinsCheckbox.addEventListener('change', function() {
             toggleHandinAutolab(disableHandinsCheckbox);
         });
     });
 </script>
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
<script>
function toggleHandinAutolab(disable_button) {
const handinField = document.querySelector('.handin-filename-field');
handinField.disabled = disable_button.checked;
if (!handinField.disabled && !handinField.value) {
handinField.value = 'handin.c';
}
}
document.addEventListener('DOMContentLoaded', function() {
const disableHandinsCheckbox = document.querySelector('.disable-handins-toggle');
toggleHandinAutolab(disableHandinsCheckbox);
disableHandinsCheckbox.addEventListener('change', function() {
toggleHandinAutolab(disableHandinsCheckbox);
});
});
</script>
<script>
const DEFAULT_FILENAME = 'handin.c';
function toggleHandinAutolab(disable_button) {
if (!disable_button) {
console.error('Invalid disable button parameter');
return;
}
const handinField = document.querySelector('.handin-filename-field');
if (!handinField) {
console.error('Handin filename field not found');
return;
}
handinField.disabled = disable_button.checked;
if (!handinField.disabled && !handinField.value) {
handinField.value = DEFAULT_FILENAME;
}
}
document.addEventListener('DOMContentLoaded', function() {
const disableHandinsCheckbox = document.querySelector('.disable-handins-toggle');
if (!disableHandinsCheckbox) {
console.error('Disable handins checkbox not found');
return;
}
toggleHandinAutolab(disableHandinsCheckbox);
disableHandinsCheckbox.addEventListener('change', function() {
toggleHandinAutolab(disableHandinsCheckbox);
});
});
</script>

<% end %>
<%# Initialize datepickers by defining linked relationships (using IDs) %>
<%= f.datetime_select :start_at,
Expand Down Expand Up @@ -29,14 +46,19 @@
help_text: "Autolab has not been configured to use Github integration. Please contact your
Autolab admin to perform the necessary setup and configuration based on the instructions on our docs." %>
<% end %>
<% unless f.object.disable_handins %>
<%= f.text_field :handin_filename, help_text: "The suffix that is appended to student submission files.
Autolab stores submission files in the handin directory as email/version_fname", placeholder: "E.g. mm.c" %>
<% end %>
<%= f.text_field :max_size, help_text: "The maximum size that a handin file can have in megabytes (MB)." %>
<%= f.check_box :disable_handins,
help_text: "Check this to disallow handins through Autolab. This option can be used to track scores
for assignments that are not submitted through Autolab such as midterms and written assignments." %>
display_name: "Disable Autolab submissions",
help_text: "Check this to disallow handins through Autolab. This option can be used to track scores for assignments that are not submitted through Autolab such as midterms and written assignments.",
class: "disable-handins-toggle",
onchange: "toggleHandinAutolab(this)" %>
<%= f.text_field :handin_filename,
value: f.object.handin_filename.presence || "handin.c",
help_text: "The suffix that is appended to student submission files. Autolab stores submission files in the handin directory as email/version_fname",
placeholder: "E.g. mm.c",
class: "handin-filename-field",
pattern: "^[a-zA-Z0-9._]+$",
title: "Only alphanumeric characters, dots, and underscores are allowed." %>
<%= f.text_field :max_size, help_text: "The maximum size that a handin file can have in megabytes (MB)." %>

<div class="action_buttons">
<%= f.submit "Save", name: "handin" %>
Expand Down
2 changes: 1 addition & 1 deletion app/views/assessments/show.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,7 @@
<div class="card-content">
<p class="valign-wrapper">
<i class="material-icons valign">file_upload_off</i>
<b class="red-text">&nbsp;Handins are disabled for this assessment.</b>
<b class="red-text">&nbsp;Autolab handins are disabled for this assessment.</b>
</p>
</div>
</div>
Expand Down