[ALT-10963] Change check_profile , opening project view.#39
Conversation
check_profile from server with an empty string.check_profile , opening project view.
meanmail
left a comment
There was a problem hiding this comment.
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
StudentTaskChangeApplierstill overwrites a non-emptycheckProfilewith an empty value when the deserialized item carries"". ThenewCheckProfile.isNotEmpty()guard from3766a0dwas dropped ine44e4073. 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, everyapplyChanges, every Retrofit request body, every deserialization). At minimum downgrade them todebug—LOG.infohere 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()matchessection.presentableName == "Topics"— fragile; reuseHyperskillCourse.getTopicsSection()and compare by reference.HyperskillSubmitConnectoruser-facing error string is hardcoded English instead ofEduCoreBundle.HyperskillSubmissionFactory.createEduTaskSubmissionnow explicitly assignsreply.checkProfile = "".EduTaskhas no check profile concept — please confirm the server expects this field on plain edu submissions; otherwise drop the assignment.RemoteEduTaskYamlMixin.ktis missing a final newline; two blank lines beforeprivate val LOGinRemoteEduTask.kt.
meanmail
left a comment
There was a problem hiding this comment.
Thanks for the quick turnaround! All blocking and important comments from my previous review are addressed in 0ecdc84 — the isNotEmpty() guard is back in StudentTaskChangeApplier, two targeted regression tests cover the empty/non-empty cases, all the info-level diagnostic logs are gone (Retrofit body, model setter, per-deserialization, per-save, per-applyChanges), unused imports in YamlLoader are dropped, isHyperskillTopicsLesson now uses HyperskillCourse.getTopicsSection() by reference, the hardcoded English error string is replaced with a new EduCoreBundle key, and the spurious reply.checkProfile = "" in createEduTaskSubmission is removed. Final newlines also fixed. LGTM. (Couldn't resolve the inline threads myself — missing permission — please mark them as resolved in the UI.)
Summary
check_profilefor remote tasks during YAML serialization/deserialization and student task updates, so server data is not overwritten with an empty valueTask.getDir()cannot resolve the task directorycheck_profileis empty and add logging around load/apply/save/submit flows to make these failures diagnosableDetails
RemoteEduTaskand explicitly serializecheck_profilecheck_profilefrom generic additional-property serialization to avoid duplicate/conflicting YAML outputcheck_profileonly for remote tasks and log mismatchesRemoteYamlLoadingExceptiongetDir()lookupTesting