Adding files that are required for jupyterlab submit button extension… by TianshuLuan-UofT · Pull Request #7986 · MarkUsProject/Markus

TianshuLuan-UofT · 2026-06-03T16:02:19Z

Linked to pull request at MarkUsProject/markus-jupyter-extension#3

Proposed Changes

Added necessary files for the JupyterLab extension (submit button) to work in the MarkUs server

Type of Change

(Write an X or a brief description next to the type or types that best describe your changes.)

Type	Applies?
🚨 Breaking change (fix or feature that would cause existing functionality to change)
✨ New feature (non-breaking change that adds functionality)	x
🐛 Bug fix (non-breaking change that fixes an issue)
🎨 User interface change (change to user interface; provide screenshots)
♻️ Refactoring (internal change to codebase, without changing functionality)
🚦 Test update (change that only adds or modifies tests)
📦 Dependency update (change that updates a dependency)
🔧 Internal (change that only affects developers or continuous integration)

Checklist

(Complete each of the following items for your pull request. Indicate that you have completed an item by changing the [ ] into a [x] in the raw text, or by clicking on the checkbox in the rendered description on GitHub.)

Before opening your pull request:

[ x] I have performed a self-review of my changes.
- Check that all changed files included in this pull request are intentional changes.
- Check that all changes are relevant to the purpose of this pull request, as described above.
I have added tests for my changes, if applicable.
- This is required for all bug fixes and new features.
I have updated the project documentation, if applicable.
- This is required for new features.
If this is my first contribution, I have added myself to the list of contributors.

After opening your pull request:

I have updated the project Changelog (this is required for all changes).
I have verified that the pre-commit.ci checks have passed.
I have verified that the CI tests have passed.
I have reviewed the test coverage changes reported by Coveralls.
I have requested a review from a project maintainer.

Questions and Comments

(Include any questions or comments you have regarding your changes.)

…. Linked to pull request at MarkUsProject/markus-jupyter-extension#3

for more information, see https://pre-commit.ci

coveralls · 2026-06-03T16:27:49Z

Coverage Report for CI Build 26897103316

Coverage decreased (-0.2%) to 90.005%

Details

Coverage decreased (-0.2%) from the base build.
Patch coverage: 111 uncovered changes across 1 file (27 of 138 lines covered, 19.57%).
No coverage regressions found.

Uncovered Changes

File	Changed	Covered	%
app/controllers/api/jupyter_submissions_controller.rb	134	23	17.16%
Total (3 files)	138	27	19.57%

Coverage Regressions

No coverage regressions found.

Coverage Stats


Relevant Lines:	50218
Covered Lines:	46179
Line Coverage:	91.96%
Relevant Branches:	2226
Covered Branches:	1023
Branch Coverage:	45.96%
Branches in Coverage %:	Yes
Coverage Strength:	122.37 hits per line

💛 - Coveralls

david-yz-liu · 2026-06-10T20:27:02Z

+
+require 'json'
+
+module Api


About naming and organization:

The new route and controller method should be under a new namespace. I suggest app/controllers/jupyter/submissions_controller.rb, with a corresponding route under the jupyter (not api).

I suggest calling the route submit rather than create (though it can still be a POST action).

david-yz-liu · 2026-06-10T20:27:56Z

+    skip_verify_authorized only: :create
+
+    def create
+      payload = request.request_parameters.presence || params.to_unsafe_h


Don't do this; use params

david-yz-liu · 2026-06-10T20:29:44Z

 # Handle Cross-Origin Resource Sharing (CORS) in order to accept cross-origin Ajax requests.

 # Read more: https://github.com/cyu/rack-cors
+# frozen_string_literal: true


Delete this

david-yz-liu · 2026-06-10T20:30:37Z

-    # Add hosts running jupyterhub to the Settings.jupyter_server.hosts settings option.
-    origins(*Settings.jupyter_server.hosts)
-    resource %r{/api/courses/\d+/assignments/\d+/submit_file},
+    origins 'http://localhost:8889', 'http://127.0.0.1:8889'


This should not be changed. Instead you can change the actual setting value, which is stored in settings.yml

david-yz-liu · 2026-06-10T20:32:40Z

@@ -0,0 +1,102 @@
+# frozen_string_literal: true
+
+require 'tempfile'


This require is unnecessary (Tempfile is already loaded and accessible)

david-yz-liu · 2026-06-10T21:01:22Z

+            'Could not find or create a grouping for this student and assignment.'
+    end
+
+    def notebook_content_as_string(content)


This is unnecessary (and somewhat overly restrictive). Whatever file data is returned from JupyterHub should be streamed directly into a tempfile that's stored as a submitted file in MarkUs. No parsing is necessary, and in fact undesirable. In the future we may wish to allow other types of files to be submitted to MarkUs from JupyterHub.

david-yz-liu · 2026-06-10T21:01:33Z

+
+      raise ArgumentError, 'Destination filename is missing.' if filename.blank?
+
+      unless filename.end_with?('.ipynb')


remove this check (see my above comment)

david-yz-liu · 2026-06-10T21:01:49Z

+    # SubmissionsHelper#upload_file expects this helper method to exist.
+    # MainApiController has it, but this controller inherits from ApplicationController
+    # to avoid the MarkUs API permission layer for the local prototype.
+    def has_missing_params?(required_params)


Delete this method (see my above comment about not using upload_file

david-yz-liu · 2026-06-10T21:03:02Z

+  end
+
+  def hub_user_uri
+    hub_origin = ENV.fetch('JUPYTERHUB_API_ORIGIN', nil).presence || @origin


Don't using an environment variable; instead, create a new setting (under the jupyter_server namespace)

david-yz-liu · 2026-06-10T21:03:25Z

+  end
+
+  def normalize_origin(origin)
+    overridden = ENV.fetch('JUPYTER_FETCH_ORIGIN', nil)


See above comment (though right now I'm not sure why there are two different environment variables)

for more information, see https://pre-commit.ci

david-yz-liu · 2026-07-07T19:09:58Z

@@ -0,0 +1,334 @@
+# frozen_string_literal: true


Delete this file (as you've created a new file, app/controllers/jupyter/jupyter_submissions_controller.rb)

david-yz-liu · 2026-07-07T19:10:27Z

+  def username
+    # Local development fallback only.
+    # This allows standalone JupyterLab testing where there is no JupyterHub identity endpoint.
+<<<<<<< HEAD


These markers indicate a merge conflict that needs to be resolved

david-yz-liu · 2026-07-07T19:28:47Z

+      user = find_user_from_jupyter_token!(jupyter_info)
+      course = find_course_from_payload!(payload)
+
+      student = course.roles.find_by(user_id: user.id)


Use course.students here to query only students

david-yz-liu · 2026-07-07T19:46:22Z

+
+module Jupyter
+  class SubmissionsController < ApplicationController
+    # Local prototype only.


I think this comment needs to be updated, and verify_authenticity_token only applies to Api controller methods, so won't apply here

david-yz-liu · 2026-07-07T19:47:05Z

+      jupyter_path = payload['notebook_path']
+
+      destination_path = sanitize_destination_path(
+        payload['destination_path'].presence ||


I think there are still more options than necessary. I want to keep things slim here; please just make notebook_path required and derive the filename from that, rather than also including destination_path and notebook_name

david-yz-liu · 2026-07-07T19:49:55Z

+        end
+      end
+
+      # TODO:


Flagging this TODO as something that still is pending

david-yz-liu · 2026-07-07T19:51:05Z

+
+    uri = contents_uri
+
+    Rails.logger.info("[JupyterNotebookFetcher] origin=#{@origin}")


Delete this logging statements

david-yz-liu · 2026-07-07T19:54:11Z

+    def submit
+      payload = params
+
+      jupyter_info = payload['jupyter'] || {}


The JupyterNotebookFetcher and JupyterIdentityFetcher are both doing the same URL normalization and validation checks. Rather than repeat them in those services, process jupyter_info here and report an error if there are issues.

david-yz-liu · 2026-07-07T19:57:25Z

+  end
+
+  def normalize_origin(origin)
+    # Browser/Jupyter may report origin as http://localhost:8889.


This adds more complication than I think is warranted (including the environment variable).

Instead, when testing this functionality you should be able to use a domain of http://host.docker.internal:3000 directly to access MarkUs (instead of localhost:3000). Unless I'm missing something about what this is referring to?

david-yz-liu · 2026-07-07T19:57:52Z

+
+# Writes a fetched Jupyter file into the student's MarkUs repository using the
+# same helper methods used by the normal MarkUs web submission flow.
+class JupyterSubmissionWriter


Is this class still being used? If not please delete the file

Tianshu Luan and others added 2 commits June 3, 2026 11:46

Adding files that are required for jupyterlab submit button extension…

305c82b

…. Linked to pull request at MarkUsProject/markus-jupyter-extension#3

[pre-commit.ci] auto fixes from pre-commit.com hooks

b39bb0c

for more information, see https://pre-commit.ci

david-yz-liu reviewed Jun 10, 2026

View reviewed changes

Tianshu Luan and others added 3 commits June 16, 2026 12:19

Revised with respect to the review suggestions.

03a0161

[pre-commit.ci] auto fixes from pre-commit.com hooks

5cd6f25

for more information, see https://pre-commit.ci

Merge branch 'master' into submit_button

7ac525e

david-yz-liu reviewed Jul 7, 2026

View reviewed changes

		@@ -0,0 +1,102 @@
		# frozen_string_literal: true

		require 'tempfile'


		raise ArgumentError, 'Destination filename is missing.' if filename.blank?

		unless filename.end_with?('.ipynb')

+                      end
+                    end
+                    # TODO:


		uri = contents_uri

		Rails.logger.info("[JupyterNotebookFetcher] origin=#{@origin}")

Uh oh!

Conversation

TianshuLuan-UofT commented Jun 3, 2026

Proposed Changes

Type of Change

Checklist

Questions and Comments

Uh oh!

coveralls commented Jun 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage Report for CI Build 26897103316

Coverage decreased (-0.2%) to 90.005%

Details

Uncovered Changes

Coverage Regressions

Coverage Stats

💛 - Coveralls

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

coveralls commented Jun 3, 2026 •

edited

Loading