feat: Implement viewer service module with auto-enrolment tracking - #52
Open
aimansharief wants to merge 31 commits into
Open
feat: Implement viewer service module with auto-enrolment tracking#52aimansharief wants to merge 31 commits into
aimansharief wants to merge 31 commits into
Conversation
Viewer module (actors + thin controllers) wired into the lern monolith: - View lifecycle (view/start|update|end|read), assessment submit/read, summary, and the recursive nested rollup in ViewerAggregatorActor. - LP auto-enrolment: structural detection via trackablenodes, per-learner optionality, level-gated progressive enrol, cert + durable user_skills at completion. Adapters make legacy content/state + activity/agg thin translators to the viewer, with deployment_mode-driven transport (monolith: in-JVM ask/tell; distributed: HTTP). Correctness fixes in this pass: - writeAllNodeEnrolments matches rows on collectionid AND this LP's contextid, so independent same-course enrolments are no longer clobbered (design §4). - Level completion derived from persisted course-enrolment status (recompute-safe). - Optionality computed once via an in-JVM memo (no DB column). - creditSkills writes only when there are new skills. - aggregate() failures are caught and logged (fire-and-forget safety). Config: viewer_enabled, deployment_mode, viewer_service_base_url, viewer_ask_timeout_ms, hierarchy_relations_cache_ttl. Migrations: viewer.cql (prod in-place) + viewer-test-keyspace.cql (fresh keyspace for testing).
Untrack viewer.cql (prod migration) and viewer-test-keyspace.cql and gitignore them; they remain on disk but are not versioned.
- CourseEnrolmentActor: drop enrolTrackableDescendants/readCollectionHierarchy/ collectTrackable (the eager pre-enrol the level-gated walker replaces) + now-unused isViewerEnabled/cassandraOperation/jsonMapper fields. Root enrol only; systemEnroll (the LP auto-enrol seam) kept. - ProgressionPolicy: remove unused hasNestedTrackable/isAssessment/computeAchievedSkills (+ orphaned privates); keep levelOf/coursesOfLevel/orderedLevels/computeOptionalNodes. - ViewerRequestKeys: canonical collectionId/contextId/contentId only (no legacy courseId/batchId fallback — callers resolve). Fix triggerAggregation to send contextId. - ContentConsumptionActor: drop redundant batchId key (viewer reads contextId).
…tests into one spec
Enable the viewer to run as its own deployable service (deployment_mode=distributed) alongside the monolith path (viewer-actors in lern-service, unchanged): - build/viewer/Dockerfile (Play dist viewer-service-1.0-SNAPSHOT). - build-local.sh / docker-build.sh: add 'viewer' service (profile=viewer, module=modules/viewer/service, dist=viewer-service-1.0-SNAPSHOT-dist.zip). - deploy.yml: gated Build/Push Viewer Service steps (VIEWER_SERVICE_BUILD var) -> lern-viewer-service image. - pr-checks.yml: viewer-build job (compile+test both viewer submodules) wired into sonar aggregation. Verified: mvn -P viewer reactor (19 modules) + play2:dist produce the dist zip.
feat (viewer) : Implement viewer service module with auto-enrolment tracking
…contextid under viewer_enabled Gated remap (Util.toCollectionColumns — no-op when viewer disabled) so the legacy enrol path works against the generalised (viewer) schema. Applied in CourseBatchDaoImpl (all methods), UserCoursesDaoImpl (read/update/updateV2/listEnrolments/getBatchParticipants), and CourseEnrolmentActor.upsertEnrollment (insert row).
…index Point the courseBatch ES read/write at the generalised field names (course-batch-1): - CourseBatchUtil.esCourseMapping writes collectionId/contextId (batch create/update indexing) - CourseBatchUtil.validateCourseBatch reads collectionId - BaseEnrolmentActor.getBatches filters by contextId - CourseEnrolmentActor.addBatchDetails keys batch docs by contextId - CourseBatchController.search translates client courseId/batchId filters to collectionId/contextId
…abled Non-viewer deployments keep courseId/batchId (old course-batch index); viewer deployments use collectionId/contextId (course-batch-1). Gated via Util.VIEWER_ENABLED in: - CourseBatchUtil.esCourseMapping (write) + validateCourseBatch (read) - BaseEnrolmentActor.getBatches (search filter) + CourseEnrolmentActor.addBatchDetails (doc key) - CourseBatchController.search (client filter translation) Adds Util import to BaseEnrolmentActor.scala and CourseBatchController.java.
When Util.VIEWER_ENABLED, translate stored viewer fields (collectionId/contextId) back to the API contract (courseId/batchId). Changes added in SearchHandlerActor and CourseEnrolmentActor to remap keys so clients and downstream logic (participants, status) remain unchanged. Also adjust batchMap construction to use JsonKey.BATCH_ID. Status enrichment from dates is preserved.
…xtId Adds collectionid=collectionId and contextid=contextId to the Cassandra column->field mapping so viewer-renamed columns resolve to the camelCase field names used by ES sync, batch search, and the enrolment list. Inert on non-viewer clusters (columns absent).
Viewer-renamed columns read back as collectionId/contextId; remap them to the API contract courseId/batchId at the enrolment read point so the list flow (course/batch details join) resolves. Gated on viewer_enabled.
listEnrolments returns null (not empty) when a user has no enrolments; move the viewer collectionId/contextId->courseId/batchId remap inside the existing isNotEmpty guard to avoid NPE.
Logs cache decision + hit/miss, raw read count and row keys, post-remap courseId/batchId, active-filter count, content-search request/result, course-filter kept count, batch join, and final returned count -- to pinpoint where /user/courses/list drops enrolments.
Logs confirmed the read returns lowercase collectionid/contextid when the column-mapping properties aren't deployed, so the camelCase-only remap left courseId/batchId null and the list dropped every enrolment. Handle both camelCase and lowercase forms.
Add idempotent CourseBatchUtil.toEsCollectionFields and call it in syncCourseBatchForeground (covers cert add/remove) and BackgroundJobManager.updateCourseBatchInfoToEs, so every courseBatch ES write emits camelCase collectionId/contextId under viewer. Prevents a secondary writer from overwriting the create doc with mismatched field names (the 'batch shows then disappears' bug).
…d rename Revert the viewer collection-identity generalisation across shared lern code and the viewer module to the canonical courseId/batchId (courseid/ batchid columns, course_id/batch_id for assessment_aggregator). Removes Util.VIEWER_ENABLED/toCollectionColumns, CourseBatchUtil.toEsCollectionFields, the esCourseMapping/SearchHandlerActor/CourseBatchController remaps, the enrolment read remaps, and the assessment column gating. Viewer actors + controllers now speak courseId/batchId. user_activity_agg.context_id and the isViewerEnabled delegation toggle are unchanged. Adds a clean courseid/batchid test-keyspace CQL (viewer*.cql untouched).
1) writeAllNodeEnrolments read row.get("courseid")/"batchid" but createResponse
returns camelCase courseId/batchId, so nodeId was null and the root enrolment
was never updated -> course completion never landed on user_enrolments. Read
camelCase (lowercase fallback).
2) Stamp user_enrolments.lastcontentaccesstime/lastreadcontentid/lastreadcontentstatus
on every view op (start/update/end/assess) so summary/list reflects real access.
- summaryDownload CSV read lowercase keys (courseid/completionpercentage/completedon) but createResponse returns camelCase, so those columns were blank. Map csv headers to the camelCase result keys. - writeAllNodeEnrolments: createResponse rows are always camelCase, so drop the unnecessary lowercase orElse fallback and read courseId/batchId directly.
- summaryDownload (format=csv) writes CSV to a temp file and uploads via the generic CloudStorageUtil (StorageServiceFactory), returning result.url per the design. Provider/container/prefix all config-driven (sunbird_cloud_service_provider, sunbird_content_cloud_storage_container, viewer_summary_upload_path); no new dep. - CloudStorageUtil: pass cloud_storage_region to StorageConfig (AWS S3 outside us-east-1); no-op when blank so Azure/existing behaviour is unchanged. Add JsonKey.CLOUD_STORAGE_REGION + config default. - viewer/service pom: add azure/aws/gcp/oci profiles so the standalone viewer dist bundles the CSP runtime jar.
Critical: - ViewConsumptionActor.touchEnrolmentAccess: read enrolment first, skip if absent (updateRecordV2 ifExists is a no-op -> would upsert phantom rows). - ViewerAggregatorActor: merge contentstatus into the existing map instead of replacing, so nodes outside the root-keyed read aren't clobbered. - ContentConsumptionActor.delegateAssessmentsToViewer: apply the legacy batch-validity guard (invalid/completed batches -> BATCH_NOT_EXISTS / NOT_A_ON_GOING_BATCH); only valid ongoing batches dispatch. Important: - ViewerSummaryActor.summaryDelete: read camelCase courseId/batchId. - ActorStartModule: bind aggregator with pekko.actor.viewer-dispatcher. - Delete dead RequestValidator.java and filters/LoggingFilter.java. - ViewController/ViewSummaryController: validate mandatory fields before dispatch (400 with offending field instead of NPE/500). - ContentConsumptionActor.updateConsumption: build finalContentList only in the legacy branch (dead work on the viewer-on path). Perf: - ViewerAggregatorActor.advanceLp: read enrolments once into a (courseId,batchId)->status snapshot; collapses the LP path's O(courses) status queries into one. Safe: runs after writeAllNodeEnrolments commits.
…tentstatus merge) The C1 fix and the courseId/batchId revert had left the suite red (tests were not re-run): touchEnrolmentAccess's getRecordByIdentifier was unstubbed, and the aggregator guard tests sent stale collectionId/contextId keys. Restore green + pin the two fixes. - ViewConsumptionActorTest: stub the enrolment read in every write-op test; add touchEnrolmentAccess cases — no stamp when the enrolment is absent (no updateRecordV2 -> phantom-row guard), stamp exactly once when present. - ViewerAggregatorActor: extract the contentstatus merge into a pure companion mergeContentStatus (updateRecordV2 replaces the whole column, so a root-keyed rollup must merge, not clobber). - ViewerAggregatorActorTest: fix guard-test request keys; add mergeContentStatus cases (preserve existing leaves, fresh wins on conflict, null-tolerant). 19/19 green.
…urseId/batchId cleanup - CourseEnrolmentActor: remove systemEnroll (and its onReceive route); LP progression now uses the standard `enrol` op. No parallel enrol method. - ViewerAggregatorActor: internalEnrol dispatches `enrol` with requestId="system" (recorded as addedBy via context REQUEST_ID); re-enrol safety stays the caller's (advanceLp only enrols courses absent from the snapshot). Skill-derivation stub comments point at the framework last-category contract (design §6), not se_skills. - CourseBatchManagementActor: nested-trackable child batches inherit the LP root batch's cert_templates (strip removed) -> each child course issues the LP cert. - ActivityAggregatorActor: viewer-dispatch payload uses courseId/batchId (was the leftover collectionId/contextId, which the viewer never reads). - ContentConsumptionActor: drop the leftover collectionId alias from content-state read (courseId is canonical); update CourseConsumptionActorTest expectation.
…trap
- Consistent fixed-phrase loggers (stage=view/rollup/lp/summary, key=val) at each
state transition: view start/update/end/access, rollup start/LP-detect/nodes/
node-complete->cert, lp optionality/level-opened/enrol/complete, summary ops.
- CourseEnrolmentActor: log an enrol confirmation ("enrol: enrolled ...").
- Wire the enrol->LP bootstrap (design Step 3): fireLpBootstrap after a user enrol
fires the viewer rollup once (monolith in-JVM tell to viewer-aggregator-actor;
distributed HTTP /v1/view/agg), so advanceLp opens the first LP course at enrol
time. Skips system-lp child enrols and no-ops when viewer disabled. enroll's
core logic unchanged.
- Fix stale "system-enrol requested" log wording in ViewerAggregatorActor.
ViewerAggregatorActor: course ancestors are not published, so derive a course's ancestor chain from one of its leaf nodes (use getLeafNodes(...).headOption -> getAncestors; fallback to empty list). Updated ProgressionPolicySpec with two tests to cover leaf ancestor chains and courses directly under root to ensure levelOf picks the correct level in both cases.
aimansharief
changed the title
- LpPolicyUtil: cached /v3/search per LP + long-TTL framework->last-category-code cache; policy/courseMeta/isAssessment/questionSets/skillsOfQuestions (reads <category>, not se_<category>Ids) - ViewerAggregatorActor: delegate policy/assessment/skill stubs; pre-assessment = first-level assessment course; Adaptive w/o pre-assessment warns+exits; PriorLearning waives prior-completed (any-batch status=2); skillsFromAssessment from assessment_aggregator best attempt (fully-correct). Engine & creditSkills unchanged - ProgressionPolicy.computeOptionalNodes: +priorCompleted - cache TTL config keys
…ts fix - ViewerAggregatorActor: extract LP progression to injectable LpProgressionEngine (SRP/DIP) + EnrolDispatcher trait (OCP, monolith/HTTP); actor 445->288 lines - A: precompute course->level map once; completedCourses Set; LpMeta.categoryCode (drop framework re-resolve) - LpPolicyUtil.searchByIds: mapper-built JSON (escaping) - CourseBatchManagementActor.batchExists: reuse readById, narrow catch so a transient DB error isn't misread as absent (no duplicate child batch) - Adaptive LP with no pre-assessment now halts (opens nothing) instead of opening the first course - Tests: LpProgressionEngineSpec (Strict/waiver/Adaptive-halt) + levelByCourse
…rse cert toggle - Bridge to the LP root only on a course's not-complete->complete transition (writeAllNodeEnrolments returns the transitioned set; gate the bridge on it). - Resolve the parent LP from the authoritative course_batch (by batchid), picking the row whose courseid differs from the completed course; extracted to a pure resolveParentLp for testability. Standalone (colon-free) batch -> no trigger. - course_certificate_enabled toggle (default true) gating per-course certs; LP cert unaffected. Root progress/status + LP cert on LP completion in the engine. - Tests: resolveParentLp (standalone/child/self-only/tiebreak) + parseCourseCertEnabled.
Update ViewerAggregatorActor so completedNow collects nodes whose status == 2 after the write pass (not only nodes that transitioned this pass). This makes LP bridging/advance run for already-complete courses (idempotent), while certificate issuance remains gated to the actual transition (status != 2 -> 2) to avoid duplicate cert events. Clarify related comments to reflect the changed semantics.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes # (issue)
Type of Change
Microservice(s) Affected
userorg-servicelms-servicenotification-servicelern-service(Unified/Common)core/shared-utilsHow Has This Been Tested?
mvn testfor the affected module.Checklist: