Check task overwriting through ProFormA

[MrSerth]

When pushing an existing task through our ProFormA API, one gets ask whether the existing task should be overwritten (given sufficient permissions) or whether a new copy should be created. That's fine and working as expected.

My questions:

• Is overwriting the task keeping or resetting further data previously set in CodeHarbor that is not sent through ProFormA? For example, I am referring to
  • labels
  • groups
  • the license selected
  • collections
  • comments
  • ratings
  • the visibility level
  • the parent UUID
• How is CodeOcean behaving there with regards to the CodeOcean-specific data:
  • tips
  • tags
  • LTI parameters
  • collections
  • proxy exercises
  • learner feedback
  • learner submissions, test runs
  • RfCs

I was unsure (haven't tested it myself) and would like to know how the systems behave. If any of these data gets lost, I would strongly suggest to keep this "additional" data, only overwriting those "attributes" sent through the API.

[kkoehn]

In CodeHarbor we work with assign_attributes

      @task.assign_attributes(
        user: @user,
        title: @proforma_task.title,
        description:,
        internal_description: @proforma_task.internal_description,
        programming_language:,
        uuid:,
        parent_uuid: @proforma_task.parent_uuid,
        language: @proforma_task.language,
        meta_data: @proforma_task.meta_data,

        submission_restrictions: @proforma_task.submission_restrictions,
        external_resources: @proforma_task.external_resources,
        grading_hints: @proforma_task.grading_hints,

        tests:,
        model_solutions:,
        files: files.values # this line has to be last, because tests and model_solutions have to remove their respective files first
      )

Therefore all mentioned relations stay in place and only attributes get changed. parent_uuid will get overwritten, because it's part of proforma. access_level is not touched and will also stay the same. For some reason user is overwritten - I would suggest removing that part, because I don't see a reason to change the ownership of a task because of an import.

[kkoehn]

In CodeOcean we do it similarly:

      @exercise.assign_attributes(
        user: @user,
        title: @task.title,
        description: @task.description,
        public: string_to_bool(extract_meta_data(@task.meta_data&.dig('meta-data'), 'public')) || false,
        hide_file_tree: string_to_bool(extract_meta_data(@task.meta_data&.dig('meta-data'), 'hide_file_tree')) || false,
        allow_file_creation: string_to_bool(extract_meta_data(@task.meta_data&.dig('meta-data'), 'allow_file_creation')) || false,
        allow_auto_completion: string_to_bool(extract_meta_data(@task.meta_data&.dig('meta-data'), 'allow_auto_completion')) || false,
        expected_difficulty: extract_meta_data(@task.meta_data&.dig('meta-data'), 'expected_difficulty') || 1,
        execution_environment_id:,

        files:
      )

Here me extract potentially saved meta-data in our own cool format. We reassign user just like in CH.
We do, however, replace existing files by creating new ones and deleting the old ones. If any data is linked to the files, we might have to take a look at that.

[MrSerth]

Thanks for checking! That's a valuable finding and seems to be mostly okay; we probably have to change only a few minor things:

CodeHarbor:

• Overwriting the parent_uuid in general is fine. However, in conjunction with CodeOcean that's problematic since CodeOcean is not aware of any parent_uuid. Imagine I export a CodeHarbor task to CodeOcean which had a parent_uuid set. After some changes in CodeOcean, I re-export the very same task back to CodeHarbor, which would result in loosing the parent_uuid. This should be fixed, so that the parent_uuid is not lost (maybe by exporting it to CodeOcean as well?)
• Overwriting user is odd, and should be removed. We won't update the user when updating the task through the CodeHarbor UI.
• Do I get right that we are also re-creating tests, model_solutions, and files in CodeHarbor completely (and dropping the previous objects)? This might change overwrite the created_at value and:
  • Test#validity
  • Test#timeout
  • Test#testing_framework
  • ... is probably an issue with the new parent_id introduced in TaskContributions #1103 for three tests, model_solutions, and files, since these IDs might need an update, too: For a valid task, none of these values is ever set (only for a contribution), but the ID saved in other objects could refer to one of the artifacts being updated through a ProFormA import.

CodeOcean:

• Reassigning the user is not too problematic, since this mirrors the behavior for updates through the UI (where we also change the user). Not ideal, but should be addressed together, I'd say.
• Similarly to CodeHarbor, we have an issue with recreating the files: The file ID of an exercise is used several times. For one, it is used as a target in the CodeOcean::Files for the column file_id when the context_type is set to Submission (same design principle as with the parent_id in CodeHarbor). Further, it is also used in Testruns for column file_id. Potentially, RequestForComments (and LinterCheckRun) is also affected.
• Otherwise, I am not aware of other data being linked to files (they are rather linked to the exercise, which is fine).

That having said, I am afraid we probably need to change some of the behavior (mainly to keep and update the existing objects).

[MrSerth]

Note to self: As soon as overwriting is possible without negative consequences, we should re-export all former Python 3.7 exercises from CodeOcean to CodeHarbor (they have been updated to use the Python 3.8 environment now).

[MrSerth]

I've tested the behavior with #1612 and updated the original checklist based on this PR. With these changes, re-exporting a task to CodeHarbor is fine.

However, we still need to tackle the problems in CodeOcean before closing this issue.

[MrSerth]

I've updated the tasks on CodeHarbor, so that I don't forget doing so later. During this update, I noticed a remaining bug in #1612. As part of that, the update process falsely recreated some TaskFiles (changing their ID) which is not ideal but happened before introducing contributions.

[MrSerth]

While reviewing openHPI/codeocean#2538 (review), I noticed we still have an issue even after merging #1612. That should be investigated before closing this issue (see the attached comment for details).

[kkoehn]

Regarding the issue in CodeHarbor, I traced the problem to a file that was originally assigned to a test, which no longer exists, and is now being used as a task_file.

This situation causes the deletion of the test (and, consequently, the file) before upsert_files @proforma_task, @task is called. While the file is still found, the destroyed? flag is already true, which means the file's attributes are frozen.

A strange issue occurs when moving upsert_files before assign_attributes: the file gets correctly assigned to the task, and the test no longer has any files. However, when the test is removed, the file is still being deleted. I assume that #destroy is being called by assign_attributes, which does not account for intermediate changes, and instead operates directly on the database state.

I created a PR with my experiments and tests, but I was not able to find a working solution.

[MrSerth]

Thank you for this work. I was able to reproduce the issue and didn't find an optimal solution either. Nevertheless, I've made a proposal that should resolve the issue, see #1696 (comment).