Skip to content

Conversation

@adrianfish
Copy link
Contributor

@adrianfish adrianfish commented Jan 22, 2026

https://sakaiproject.atlassian.net/browse/SAK-52288

This change also normalises the test file layout, bringing the scorm test into the same package as the other tests.

Summary by CodeRabbit

  • New Features

    • Added SCORM API endpoints for session creation and runtime initialization.
  • Bug Fixes

    • SCORM API now returns HTTP 404 when a session is not found.
  • Documentation

    • Updated documentation URL path in README.
    • Added SCORM API docs with request/response examples.
  • Tests

    • Reorganized controller tests and added REST Docs integration for SCORM tests.

✏️ Tip: You can customize this high-level summary in your review settings.

https://sakaiproject.atlassian.net/browse/SAK-52288

This change also normalises the test file layout, bringing the scorm
test into the same package as the other tests.
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 22, 2026

Walkthrough

Updated documentation URL and added SCORM API docs; refactored ScormController formatting and changed getSession to return 404 when missing; reorganized test packages to org.sakaiproject.webapi.controllers.test; integrated REST Docs and test configuration into ScormControllerTest.

Changes

Cohort / File(s) Summary
Documentation
webapi/README.md, webapi/src/main/asciidoc/webapi.adoc
README docs URL updated from http://localhost/docs/webapi.htmlhttp://localhost/api/docs/webapi.html. Added SCORM API documentation blocks (Create Session, Initialize Runtime) with request/response examples and snippet inclusions.
ScormController
webapi/src/main/java/org/sakaiproject/webapi/controllers/ScormController.java
Formatting/refactor (brace and whitespace changes). Functional behavior: getSession now returns HTTP 404 Not Found when a session does not exist. No public API signatures changed.
Test package moves
webapi/src/test/java/org/sakaiproject/webapi/controllers/test/...
webapi/src/test/java/org/sakaiproject/webapi/controllers/test/AnnouncementsControllerTests.java, .../BaseControllerTests.java, .../CalendarControllerTests.java, .../LoginControllerTests.java, .../WebApiTestConfiguration.java
Updated package declarations from org.sakaiproject.webapi.testorg.sakaiproject.webapi.controllers.test. No other logic changes.
ScormControllerTest (tests & REST Docs)
webapi/src/test/java/org/sakaiproject/webapi/controllers/test/ScormControllerTest.java
Test class moved to controllers.test package, now annotated with @RunWith(SpringJUnit4ClassRunner.class) and @ContextConfiguration, extends BaseControllerTests. Integrated JUnitRestDocumentation and configured MockMvc for REST Docs; added document() calls for endpoints (e.g., create-scorm-session, initialize-runtime).

Suggested reviewers

  • ern
  • ottenhoff
🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately reflects the main changes: adding SCORM REST documentation to the webapi module, which aligns with the primary additions in webapi.adoc and ScormControllerTest.java.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
webapi/src/test/java/org/sakaiproject/webapi/controllers/test/LoginControllerTests.java (2)

106-114: Replace var with explicit types per coding guidelines.

The use of var for local variable type inference violates the project's coding guidelines. Explicit types are required for all local variable declarations.

♻️ Proposed fix
-        var auth = mock(Authentication.class);
+        Authentication auth = mock(Authentication.class);
         when(authenticationManager.authenticate(any())).thenReturn(auth);

-        var session = mock(Session.class);
+        Session session = mock(Session.class);
         when(session.getId()).thenReturn("session1");
         when(sessionManager.startSession()).thenReturn(session);

-        var username = "user1";
-        var password = "password1";
+        String username = "user1";
+        String password = "password1";

135-136: Replace var with explicit types.

Same issue as above—use explicit String type declarations.

♻️ Proposed fix
-        var username = "user1";
-        var password = "password1";
+        String username = "user1";
+        String password = "password1";
🤖 Fix all issues with AI agents
In
`@webapi/src/test/java/org/sakaiproject/webapi/controllers/test/ScormControllerTest.java`:
- Around line 153-157: The test "closeSessionReturnsNoContent" expects 204 but
ScormController.closeSession() currently returns void with no `@ResponseStatus`,
producing 200; fix by adding `@ResponseStatus`(HttpStatus.NO_CONTENT) to the
ScormController.closeSession method (to follow REST best practices) and update
the test in ScormControllerTest.closeSessionReturnsNoContent to assert
status().isNoContent() so the controller and test agree.
🧹 Nitpick comments (2)
webapi/src/test/java/org/sakaiproject/webapi/controllers/test/BaseControllerTests.java (1)

58-62: Empty catch block silently swallows exceptions.

While the fallback to the original URI is reasonable, the empty catch block loses diagnostic information that could help debugging. Consider logging the exception.

♻️ Suggested improvement
                     public URI getUri() {
                         try {
                             return new URI(uri.getScheme(), uri.getUserInfo(), uri.getHost(), uri.getPort(), "/api" + uri.getPath(), uri.getQuery(), uri.getFragment());
-                        } catch (Exception e) {
+                        } catch (Exception e) {
+                            // URI construction failed; fall back to original
                         }
                         return uri;
                     }
webapi/src/main/asciidoc/webapi.adoc (1)

69-81: Consider adding the session cookie reminder for consistency.

Other API sections (Announcements, Calendar) include __See the login api about session cookies__ to remind users about authentication. The Scorm API section would benefit from the same note.

♻️ Suggested addition
 === Scorm API

+__See the login api about session cookies__
+
 Create a Scorm session:
 include::{snippets}/create-scorm-session/http-request.adoc[]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant