Merge branch 'MarkUsProject:master' into autotester_pass_tags_comments

Changelog.md

@@ -14,6 +14,7 @@
 - Ensures row selection for peer reviewer unassigning has the same validation checks as individual selections (#7274)
 - Ensures mark levels on a rubric criterion are properly scaled when its max mark is updated (#7311)
 - Refactor contributors list in About section to read from markus-contributors.txt (#7374)
+- Optimized the querying of submissions when assigning graders (#7381)
 
 ### 🔧 Internal changes
 
@@ -27,7 +28,9 @@
 - Add unit tests for `marks_graders_controller` (#7382)
 - Convert front-end tests from enzyme to react testing library; add `@testing-library/user-event` (#7379)
 - Refactor the `Result` component and its children to use React context API (#7380)
+- Implement `contain_message` and `have_message` custom Rspec matchers to check for flash message content (#7386)
 - Update Python version to 3.13 in seed autotest schemas (#7388)
+- Rename jupyter notebook content functions and files to generalize to html content (#7391)
 
 ## [v2.6.1]
 

app/assets/javascripts/Annotations/globals.js

@@ -6,7 +6,7 @@ var ANNOTATION_TYPES = {
   CODE: 0,
   IMAGE: 1,
   PDF: 2,
-  NOTEBOOK: 3,
+  HTML: 3,
 };
 
 // Enum to tell the code if an image, code, or pdf is being shown

app/assets/javascripts/Annotations/notebook_annotations.js → app/assets/javascripts/Annotations/html_annotations.js

@@ -31,9 +31,9 @@ function check_annotation_overlap(range) {
   );
 }
 
-function get_notebook_annotation_range() {
-  const notebook_iframe = document.getElementById("notebook");
-  const target = notebook_iframe.contentDocument;
+function get_html_annotation_range() {
+  const iframe = document.getElementById("html-content");
+  const target = iframe.contentDocument;
   const selection = target.getSelection();
   if (selection.rangeCount >= 1) {
     const range = selection.getRangeAt(0);

app/assets/javascripts/Results/main.js

@@ -11,7 +11,7 @@
 //= require Annotations/text_annotation_manager
 //= require Annotations/image_annotation_manager
 //= require Annotations/pdf_annotation_manager
-//= require Annotations/notebook_annotations
+//= require Annotations/html_annotations
 
 // Page-specific Javascript
 //= require Grader/marking

app/assets/stylesheets/common/_file_viewer.scss

@@ -1,4 +1,4 @@
-.notebook {
+.html-content {
   height: 595px;
   width: 100%;
 }

app/controllers/graders_controller.rb

@@ -253,10 +253,8 @@ def unassign_graders(grouping_ids, grader_ids)
 
   # Returns array of grouping ids with non empty submissions
   def filter_empty_submissions(grouping_ids)
-    grouping_ids.select do |grouping_id|
-      submission = Submission.find_by(grouping_id: grouping_id, submission_version_used: true)
-      submission && !submission.is_empty
-    end
+    Submission.where(grouping_id: grouping_ids, is_empty: false, submission_version_used: true)
+              .pluck(:grouping_id)
   end
 
   def filter_grouping_by_section(section_assignments, assignment)

app/controllers/submissions_controller.rb

@@ -18,7 +18,7 @@ class SubmissionsController < ApplicationController
     p.frame_src(*PERMITTED_IFRAME_SRC)
   end
 
-  content_security_policy_report_only only: :notebook_content
+  content_security_policy_report_only only: :html_content
 
   def index
     respond_to do |format|
@@ -483,9 +483,9 @@ def download_file
     end
 
     if params[:show_in_browser] == 'true' && file.is_pynb? && Rails.application.config.nbconvert_enabled
-      redirect_to notebook_content_course_assignment_submissions_url(current_course,
-                                                                     record.grouping.assignment,
-                                                                     select_file_id: params[:select_file_id])
+      redirect_to html_content_course_assignment_submissions_url(current_course,
+                                                                 record.grouping.assignment,
+                                                                 select_file_id: params[:select_file_id])
       return
     end
 
@@ -561,7 +561,7 @@ def download
     nbconvert_enabled = Rails.application.config.nbconvert_enabled
 
     if FileHelper.get_file_type(params[:file_name]) == 'jupyter-notebook' && preview && nbconvert_enabled
-      redirect_to action: :notebook_content,
+      redirect_to action: :html_content,
                   course_id: current_course.id,
                   assignment_id: params[:assignment_id],
                   grouping_id: params[:grouping_id],
@@ -641,7 +641,7 @@ def download_summary
     send_data_download csv_data, filename: "#{assignment.short_identifier}_submissions.csv"
   end
 
-  def notebook_content
+  def html_content
     if params[:select_file_id]
       file = SubmissionFile.find(params[:select_file_id])
       file_contents = file.retrieve_file
@@ -672,15 +672,15 @@ def notebook_content
       filename = params[:file_name]
     end
 
-    @notebook_type = FileHelper.get_file_type(filename)
+    @file_type = FileHelper.get_file_type(filename)
     if path.nil?
-      @notebook_content = ''
+      @html_content = ''
     else
       sanitized_filename = ActiveStorage::Filename.new("#{filename}.#{revision_identifier}").sanitized
       unique_path = File.join(grouping.group.repo_name, path, sanitized_filename)
-      @notebook_content = notebook_to_html(file_contents, unique_path, @notebook_type)
+      @html_content = notebook_to_html(file_contents, unique_path, @file_type)
     end
-    render layout: 'notebook'
+    render layout: 'html_content'
   end
 
   ##

app/javascript/Components/Helpers/range_selector.js

@@ -95,7 +95,7 @@ function addMouseOverToNode(node, content) {
   node.ownerDocument.body.appendChild(content_container);
   node.addEventListener("mouseenter", e => {
     // We make MathJax typesetting lazy to avoid rendering issues when editing an annotation
-    // from the "Annotations" tab, where the NotebookViewer component is updated before being visible.
+    // from the "Annotations" tab, where the HTMLViewer component is updated before being visible.
     node.ownerDocument.defaultView.MathJax.typeset([content_container]);
     let offset_height = 0;
     for (let elem of node.ownerDocument.getElementsByClassName("markus-annotation-content")) {

app/javascript/Components/Result/file_viewer.jsx

@@ -3,7 +3,7 @@ import React from "react";
 import {ImageViewer} from "./image_viewer";
 import {TextViewer} from "./text_viewer";
 import {PDFViewer} from "./pdf_viewer";
-import {NotebookViewer} from "./notebook_viewer";
+import {HTMLViewer} from "./html_viewer";
 import {BinaryViewer} from "./binary_viewer";
 import {URLViewer} from "./url_viewer";
 
@@ -62,7 +62,7 @@ export class FileViewer extends React.Component {
     } else if (this.props.selectedFileType === "pdf") {
       return <PDFViewer annotationFocus={this.props.annotationFocus} {...commonProps} />;
     } else if (this.props.selectedFileType === "jupyter-notebook") {
-      return <NotebookViewer annotationFocus={this.props.annotationFocus} {...commonProps} />;
+      return <HTMLViewer annotationFocus={this.props.annotationFocus} {...commonProps} />;
     } else if (this.props.selectedFileType === "binary") {
       return <BinaryViewer content={this.state.content} {...commonProps} />;
     } else if (this.props.selectedFileType === "markusurl") {

app/javascript/Components/Result/notebook_viewer.jsx → app/javascript/Components/Result/html_viewer.jsx

@@ -1,7 +1,7 @@
 import React from "react";
 import {markupTextInRange} from "../Helpers/range_selector";
 
-export class NotebookViewer extends React.PureComponent {
+export class HTMLViewer extends React.PureComponent {
   constructor(props) {
     super(props);
     this.iframe = React.createRef();
@@ -14,7 +14,7 @@ export class NotebookViewer extends React.PureComponent {
   }
 
   readyAnnotations = () => {
-    annotation_type = ANNOTATION_TYPES.NOTEBOOK;
+    annotation_type = ANNOTATION_TYPES.HTML;
   };
 
   renderAnnotations = () => {
@@ -61,8 +61,8 @@ export class NotebookViewer extends React.PureComponent {
     return (
       <div>
         <iframe
-          className={"notebook"}
-          id={"notebook"}
+          className={"html-content"}
+          id={"html-content"}
           key={this.props.url}
           onLoad={this.renderAnnotations}
           src={this.props.url + "&preview=true"}

app/javascript/Components/Result/result.jsx

@@ -325,8 +325,8 @@ class Result extends React.Component {
 
   extend_with_selection_data = annotation_data => {
     let box;
-    if (annotation_type === ANNOTATION_TYPES.NOTEBOOK) {
-      const range = get_notebook_annotation_range();
+    if (annotation_type === ANNOTATION_TYPES.HTML) {
+      const range = get_html_annotation_range();
       box = {
         start_node: pathToNode(range.startContainer),
         start_offset: range.startOffset,

app/policies/submission_policy.rb

@@ -29,7 +29,7 @@ def view_files?
     true
   end
 
-  def notebook_content?
+  def html_content?
     Rails.application.config.nbconvert_enabled && check?(:view_files?)
   end
 

app/views/layouts/notebook.html.erb → app/views/layouts/html_content.html.erb

@@ -1,7 +1,7 @@
 <!DOCTYPE html>
 <html lang="<%= I18n.locale %>">
   <head>
-    <% if @notebook_type == 'jupyter-notebook' %>
+    <% if @file_type == 'jupyter-notebook' %>
       <%= stylesheet_link_tag 'notebook_common' %>
       <% if @current_user&.theme == 'dark' %>
         <%= stylesheet_link_tag 'notebook_dark' %>

app/views/results/common/_syntax_highlighter_brushes.html.erb

@@ -8,5 +8,5 @@
                            'Annotations/text_annotation_manager',
                            'Annotations/image_annotation_manager',
                            'Annotations/pdf_annotation_manager',
-                           'Annotations/notebook_annotations',
+                           'Annotations/html_annotations',
                            nonce: true %>

app/views/submissions/html_content.html.erb

@@ -0,0 +1 @@
+<%= @html_content.html_safe %>

config/routes.rb

@@ -327,7 +327,7 @@
           get 'download'
           post 'zip_groupings_files'
           get 'download_zipped_file'
-          get 'notebook_content'
+          get 'html_content'
           get 'download_summary'
           get 'repo_browser'
           post 'repo_browser'

spec/controllers/annotation_categories_controller_spec.rb

@@ -702,8 +702,7 @@
 
         expect(response).to have_http_status(:found)
         expect(flash[:error]).to be_nil
-        expect(flash[:success].map { |f| extract_text f }).to eq([I18n.t('upload_success',
-                                                                         count: 2)].map { |f| extract_text f })
+        expect(flash[:success]).to have_message(I18n.t('upload_success', count: 2))
         expect(response).to redirect_to(action: 'index', assignment_id: assignment.id)
 
         expect(AnnotationCategory.all.size).to eq(2)
@@ -729,8 +728,7 @@
 
         expect(response).to have_http_status :found
         expect(flash[:error]).to be_nil
-        expect(flash[:success].map { |f| extract_text f }).to eq([I18n.t('upload_success',
-                                                                         count: 3)].map { |f| extract_text f })
+        expect(flash[:success]).to have_message(I18n.t('upload_success', count: 3))
         expect(response).to redirect_to(action: 'index', assignment_id: assignment.id)
 
         expect(AnnotationCategory.all.size).to eq 3

spec/controllers/assignments_controller_spec.rb

@@ -2277,7 +2277,7 @@
       grouping # lazy initialization
       delete_as instructor, :destroy, params: { course_id: course.id, id: assignment.id }
       expect(Assignment.exists?(assignment.id)).to be(true)
-      expect(flash[:error]).to eq(["<p>#{I18n.t('assignments.assignment_has_groupings')}</p>"])
+      expect(flash[:error]).to have_message(I18n.t('assignments.assignment_has_groupings'))
       expect(flash.to_hash.length).to eq(1)
       expect(flash[:error].length).to eq(1)
       expect(response).to have_http_status(:found)
@@ -2286,8 +2286,8 @@
     it 'should successfully DELETE assignment (no groups)' do
       delete_as instructor, :destroy, params: { course_id: course.id, id: assignment.id }
       expect(Assignment.exists?(assignment.id)).to be(false)
-      expect(flash[:success]).to eq(I18n.t('flash.actions.destroy.success',
-                                           resource_name: assignment.short_identifier))
+      expect(flash[:success]).to have_message(I18n.t('flash.actions.destroy.success',
+                                                     resource_name: assignment.short_identifier))
       expect(flash.to_hash.length).to eq(1)
       expect(response).to have_http_status(:found)
     end
@@ -2318,10 +2318,8 @@
 
       delete_as instructor, :destroy, params: { course_id: course.id, id: assignment.id }
 
-      expect(flash[:error][0]).to include(
-        I18n.t('activerecord.errors.models.assignment_deletion',
-               problem_message: 'some error')
-      )
+      expect(flash[:error]).to contain_message(I18n.t('activerecord.errors.models.assignment_deletion',
+                                                      problem_message: 'some error'))
       expect(Assignment.exists?(assignment.id)).to be true
       expect(response).to redirect_to(edit_course_assignment_path(course.id, assignment.id))
     end

spec/controllers/automated_tests_controller_spec.rb

@@ -279,7 +279,7 @@
         end
 
         it 'flashes an error message' do
-          expect(flash[:error].join('\n')).to include(I18n.t('errors.invalid_path'))
+          expect(flash[:error]).to contain_message(I18n.t('errors.invalid_path'))
         end
 
         it 'does not save the file' do
@@ -294,7 +294,7 @@
         end
 
         it 'flashes an error message' do
-          expect(flash[:error].join('\n')).to include(I18n.t('errors.invalid_path'))
+          expect(flash[:error]).to contain_message(I18n.t('errors.invalid_path'))
         end
 
         it 'does not create the folder' do
@@ -309,7 +309,7 @@
         end
 
         it 'flashes an error message' do
-          expect(flash[:error].join('\n')).to include(I18n.t('errors.invalid_path'))
+          expect(flash[:error]).to contain_message(I18n.t('errors.invalid_path'))
         end
 
         it 'does not delete the folder' do
@@ -324,7 +324,7 @@
         end
 
         it 'flashes an error message' do
-          expect(flash[:error].join('\n')).to include(I18n.t('errors.invalid_path'))
+          expect(flash[:error]).to contain_message(I18n.t('errors.invalid_path'))
         end
 
         it 'does not delete the file' do

spec/controllers/courses_controller_spec.rb

@@ -367,8 +367,7 @@
       test2 = Assignment.find_by(short_identifier: @test_asn2)
       expect(test2).not_to be_nil
       expect(flash[:error]).to be_nil
-      expect(flash[:success].map { |f| extract_text f }).to eq([I18n.t('upload_success',
-                                                                       count: 2)].map { |f| extract_text f })
+      expect(flash[:success]).to have_message(I18n.t('upload_success', count: 2))
       expect(response).to redirect_to(course_assignments_path(course))
     end