Skip to content

Conversation

@hansegucker
Copy link
Collaborator

Contains only smaller changes of #2500

Copy link
Member

@niklasmohrin niklasmohrin left a comment

Choose a reason for hiding this comment

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

Looks good; can you make the PR title and comment more descriptive so that we have something more to read in the Git history

richardebeling
richardebeling previously approved these changes Oct 13, 2025
@hansegucker hansegucker changed the title JSON enrollment importer v2 - part 1 JSON enrollment importer v2 - part 1 - skip certain course types, use callingname, set is_rewarded correctly, and fix edit rights for o Oct 20, 2025
@hansegucker hansegucker changed the title JSON enrollment importer v2 - part 1 - skip certain course types, use callingname, set is_rewarded correctly, and fix edit rights for o JSON enrollment importer v2 - part 1 - skip certain course types, use callingname, set is_rewarded correctly, and fix edit rights for contributors Oct 20, 2025
@hansegucker hansegucker force-pushed the json-enrollment-importer-v2 branch 2 times, most recently from 1794a93 to fdb089a Compare October 20, 2025 16:17
richardebeling
richardebeling previously approved these changes Oct 20, 2025
Copy link
Member

@janno42 janno42 left a comment

Choose a reason for hiding this comment

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

Manually tested, seems to work :)

Small things you could fix here although they are from v1:

  • Newlines between log entries are not consistent: For example after the "Name Changes" block there is no newline before "New Courses"
  • Course names sometimes begin with whitespace in the log - the cleaned name should be shown instead.

@richardebeling richardebeling self-requested a review November 3, 2025 18:47
@janno42
Copy link
Member

janno42 commented Nov 3, 2025

Two more wishes: 🧞

  • Replace all instances of continuous white space with a single space character (in all fields)
  • types of exam events have changed. We now need to import exam events without a related non-exam event. In this case, their type is now the course type. They don't have a prefix in the title anymore.

@hansegucker hansegucker mentioned this pull request Nov 3, 2025
@hansegucker hansegucker force-pushed the json-enrollment-importer-v2 branch from 5d9a59c to 85841ce Compare November 10, 2025 16:56
janno42
janno42 previously approved these changes Nov 10, 2025
Copy link
Member

@janno42 janno42 left a comment

Choose a reason for hiding this comment

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

✅ Functionality checked

There will be future changes but we can put that in v3 :)

Copy link
Member

@niklasmohrin niklasmohrin left a comment

Choose a reason for hiding this comment

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

looks mostly good, some comments:

richardebeling
richardebeling previously approved these changes Nov 10, 2025
Copy link
Member

@richardebeling richardebeling left a comment

Choose a reason for hiding this comment

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

lgtm if the open comments are resolved.

We have a few cases where a later commit overwrites a change of an earlier commit (formatting, narrower type annotation). Not to bad, if you want to rebase-merge we can do that, but I think squash-merge would also work (and clean that up)

@hansegucker hansegucker force-pushed the json-enrollment-importer-v2 branch from 6539ca3 to a9af32c Compare November 12, 2025 16:50
@hansegucker hansegucker force-pushed the json-enrollment-importer-v2 branch from a9af32c to f619d33 Compare November 16, 2025 14:11
@janno42 janno42 requested a review from niklasmohrin November 17, 2025 16:16
@hansegucker hansegucker force-pushed the json-enrollment-importer-v2 branch from f619d33 to f2579b1 Compare November 17, 2025 16:31
Copy link
Member

@janno42 janno42 left a comment

Choose a reason for hiding this comment

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

:shipit:

@janno42 janno42 merged commit e5e53e8 into e-valuation:main Nov 17, 2025
13 of 15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants