[ALT-10963] Change `check_profile` , opening project view. by KirillovProj · Pull Request #39 · hyperskill/hyperskill-plugin

KirillovProj · 2026-04-29T14:14:55Z

Summary

preserve check_profile for remote tasks during YAML serialization/deserialization and student task updates, so server data is not overwritten with an empty value
harden Hyperskill project opening and YAML loading by tolerating missing config directories, skipping unsupported topic tasks, and handling remote YAML loading failures per item instead of aborting the whole load
fix task directory resolution in the course view so projects still open correctly when Task.getDir() cannot resolve the task directory
block remote task submission when check_profile is empty and add logging around load/apply/save/submit flows to make these failures diagnosable

Details

added a dedicated YAML mixin for RemoteEduTask and explicitly serialize check_profile
excluded check_profile from generic additional-property serialization to avoid duplicate/conflicting YAML output
changed student task change application to update check_profile only for remote tasks and log mismatches
made config-dir lookups nullable and updated YAML sync/load code to fail soft when files or directories are missing
updated deep remote info loading to continue after RemoteYamlLoadingException
adjusted course view logic to resolve visible task files from the actual task directory instead of relying on a later getDir() lookup
added regression coverage for YAML serialization, YAML change handling, and course view task directory lookup
updated the Hyperskill courses link in the new project UI to use the tracked campaign URL

Testing

added/updated regression tests for:
- remote task YAML serialization
- YAML change-after-event handling
- course view task directory lookup

^ALT-10963

meanmail

Review summary

The PR has grown well beyond its original one-line scope. The original empty-string-overwrite fix from commit 3766a0d was lost in subsequent commits, so the very bug this PR was created for is no longer prevented. There are also several leftover debug logs, unused imports, scope creep (UI URL change, project view refactor, soft-fail YAML loading, Retrofit body logging, two large docs files), and a few smaller correctness/style issues. Details inline.

Blocking

StudentTaskChangeApplier still overwrites a non-empty checkProfile with an empty value when the deserialized item carries "". The newCheckProfile.isNotEmpty() guard from 3766a0d was dropped in e44e4073. Please restore it and add a regression test covering exactly this scenario.

Strongly recommended before merge

Strip the [DEBUG_LOG]-prefixed and per-call info logs (model setter, every YAML save, every applyChanges, every Retrofit request body, every deserialization). At minimum downgrade them to debug — LOG.info here will produce a lot of noise on real courses and the Retrofit body log can leak auth-bearing payloads.
Drop unused imports in YamlLoader.kt (EduOAuthCodeFlowConnector, HTTP_BAD_REQUEST, HTTP_FORBIDDEN, HTTP_UNAUTHORIZED).
Consider splitting the unrelated changes (course-view refactor, UTM-link change, soft-fail in YamlDeepLoader, Retrofit body logging, the two new docs files) into separate PRs — they are not part of ALT-10963 and make review harder.

Smaller things

isHyperskillTopicsLesson() matches section.presentableName == "Topics" — fragile; reuse HyperskillCourse.getTopicsSection() and compare by reference.
HyperskillSubmitConnector user-facing error string is hardcoded English instead of EduCoreBundle.
HyperskillSubmissionFactory.createEduTaskSubmission now explicitly assigns reply.checkProfile = "". EduTask has no check profile concept — please confirm the server expects this field on plain edu submissions; otherwise drop the assignment.
RemoteEduTaskYamlMixin.kt is missing a final newline; two blank lines before private val LOG in RemoteEduTask.kt.

meanmail · 2026-05-12T16:13:28Z

+      val newCheckProfile = deserializedItem.checkProfile
+      if (newCheckProfile != existingItem.checkProfile) {
+        LOG.info("Updating checkProfile for task ${existingItem.name}: '${existingItem.checkProfile}' -> '$newCheckProfile'")
+        existingItem.checkProfile = newCheckProfile


Blocking — the original bug is back.

The original fix in commit 3766a0d was if (newCheckProfile.isNotEmpty()) existingItem.checkProfile = newCheckProfile. Commit e44e4073 replaced that with a plain inequality check, so now if the deserialized YAML carries check_profile: '' while the in-memory task has a real profile (e.g. "hyperskill_go"), this line still overwrites the existing value with an empty string — the exact scenario ALT-10963 is supposed to prevent. The LOG.warn("checkProfile is empty … after applying changes") below is essentially logging the bug rather than preventing it.

Suggested fix: bring back the guard, e.g.

if (newCheckProfile.isNotEmpty() && newCheckProfile != existingItem.checkProfile) { existingItem.checkProfile = newCheckProfile }

and add a regression test that loads YAML with an empty check_profile over a RemoteEduTask whose checkProfile was already set.

meanmail · 2026-05-12T16:13:28Z

  override fun applyChanges(existingItem: Task, deserializedItem: Task) {
+    LOG.info("Applying changes for task: ${existingItem.name} (id=${existingItem.id})")
+    LOG.info("Existing item checkProfile: ${(existingItem as? RemoteEduTask)?.checkProfile}")
+    LOG.info("Deserialized item checkProfile: ${(deserializedItem as? RemoteEduTask)?.checkProfile}")


Three LOG.info lines per applyChanges call will be very noisy on real courses (this runs for every YAML change event). Please downgrade to debug or drop them — the same information is already logged on the setter and on save.

meanmail · 2026-05-12T16:13:28Z

+        existingItem.checkProfile = newCheckProfile
+      }
+      if (existingItem.checkProfile.isEmpty()) {
+        LOG.warn("checkProfile is empty for RemoteEduTask ${existingItem.name} after applying changes")


Once the empty-overwrite guard from line 35 is restored, this warning becomes unreachable / misleading. Either remove it, or replace it with a hard failure (throw YamlLoadingException(...)) so we don't silently keep a task in a broken state.

meanmail · 2026-05-12T16:13:28Z

  var checkProfile: String = ""
+    set(value) {
+      if (field != value) {
+        LOG.info("[DEBUG_LOG] RemoteEduTask.checkProfile set to '$value' (was '$field')")


Putting a logging side-effect on a domain-model property setter (and with a [DEBUG_LOG] prefix) reads like leftover debugging instrumentation. If we need to trace when checkProfile changes, do it at the call sites (loader / change applier) rather than the model, and at debug level.

meanmail · 2026-05-12T16:13:29Z

 */
 // see EDU-4504 Support Go edu problems
+
+


Nit: two consecutive blank lines between the type-level comment and private val LOG.

meanmail · 2026-05-12T16:13:29Z

+    if (task.checkProfile.isEmpty()) {
+      val message = "Check profile is empty for task ${task.name}. Submission aborted to avoid server error."
+      LOG.error(message)
+      return Err(message + " Please try to 'Synchronize project' or contact support.")


User-facing string is hardcoded in English here. Other error strings in this file go through EduCoreBundle.message(...) — please follow the same convention so the message stays translatable.

meanmail · 2026-05-12T16:13:29Z

  }

  fun submitRemoteEduTask(task: RemoteEduTask, files: List<SolutionFile>): Result<StepikBasedSubmission, String> {
+    LOG.info("Submitting RemoteEduTask: ${task.name} (id=${task.id}), checkProfile='${task.checkProfile}'")


Logging every submission at info is fine for now, but please consider whether debug would be enough — every check action will land here.

meanmail · 2026-05-12T16:13:29Z

    reply.score = if (task.status == CheckStatus.Solved) "1" else "0"
    reply.solution = files
+    reply.feedback = Feedback(feedback)
+    reply.checkProfile = ""


createEduTaskSubmission is for plain EduTask, which has no notion of checkProfile. Forcing reply.checkProfile = "" here sends an empty check_profile to the server for every regular edu submission — please confirm the server tolerates it; otherwise drop the assignment. If it is needed only to satisfy some serialization contract, a comment explaining why would help.

meanmail · 2026-05-12T16:13:29Z

+    if (newRequest.body != null) {
+      val buffer = Buffer()
+      runCatching { newRequest.body!!.writeTo(buffer) }
+        .onSuccess { LOG.info("Retrofit request body for ${newRequest.method} ${newRequest.url}: ${buffer.readUtf8()}") }


Logging the full request body at info for every Retrofit call is risky: it will (a) flood idea.log and (b) potentially log payloads that include tokens/PII (this interceptor runs after the auth interceptors). At minimum downgrade to debug and gate by a system property; ideally remove before merge.

meanmail · 2026-05-12T16:13:29Z

+  @set:JsonProperty(CHECK_PROFILE)
+  @get:JsonInclude(JsonInclude.Include.ALWAYS)
+  var checkProfile: String = ""
 }


Nit: file is missing a final newline.

Don't overwrite check_profile from server with an empty string.

3766a0d

^ALT-10963

KirillovProj requested a review from Tsyklop April 29, 2026 14:14

KirillovProj self-assigned this Apr 29, 2026

KirillovProj added the bug Something isn't working label Apr 29, 2026

Tsyklop approved these changes Apr 29, 2026

View reviewed changes

Tsyklop added 3 commits May 4, 2026 15:20

add logging

254e4a1

fix opening project

e44e407

fix opening projects.

f26db60

Tsyklop changed the title ~~[ALT-10963] Don't overwrite check_profile from server with an empty string.~~ [ALT-10963] Change check_profile , opening project view. May 12, 2026

potential fix python checker error

5268dc9

meanmail suggested changes May 12, 2026

View reviewed changes

review fix

0ecdc84

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ALT-10963] Change `check_profile` , opening project view.#39

[ALT-10963] Change `check_profile` , opening project view.#39
KirillovProj wants to merge 6 commits into
mainfrom
bug/ALT-10963_empty_check_profile

KirillovProj commented Apr 29, 2026 •

edited by Tsyklop

Loading

Uh oh!

meanmail left a comment

Uh oh!

meanmail May 12, 2026

Uh oh!

meanmail May 12, 2026

Uh oh!

meanmail May 12, 2026

Uh oh!

meanmail May 12, 2026

Uh oh!

meanmail May 12, 2026

Uh oh!

meanmail May 12, 2026

Uh oh!

meanmail May 12, 2026

Uh oh!

meanmail May 12, 2026

Uh oh!

meanmail May 12, 2026

Uh oh!

meanmail May 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

KirillovProj commented Apr 29, 2026 • edited by Tsyklop Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Details

Testing

Uh oh!

meanmail left a comment

Choose a reason for hiding this comment

Review summary

Blocking

Strongly recommended before merge

Smaller things

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

KirillovProj commented Apr 29, 2026 •

edited by Tsyklop

Loading