Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[#197] Klanten API migration command #263

Merged
merged 48 commits into from
Nov 22, 2024
Merged

Conversation

SonnyBA
Copy link
Contributor

@SonnyBA SonnyBA commented Oct 11, 2024

Fixes #197

Changes

Adds a management command which allows the migration of the previous Klanten API to the Klantinteracties API. Two running instances (Klanten and Klantinteracties API) are required to do so with an API token for the Klanten API.

@SonnyBA SonnyBA self-assigned this Oct 11, 2024
@SonnyBA SonnyBA marked this pull request as ready for review October 11, 2024 14:20
@stevenbal
Copy link
Collaborator

Looks good overall, I think it might also be good to add some basic documentation on how to use this command to the docs

@SonnyBA SonnyBA force-pushed the issue/#197-migration-script branch from 4d71766 to 01f7acc Compare October 25, 2024 13:26
@stevenbal stevenbal assigned stevenbal and unassigned SonnyBA Oct 25, 2024
@stevenbal stevenbal assigned SonnyBA and unassigned stevenbal Nov 5, 2024
@stevenbal
Copy link
Collaborator

@SonnyBA I haven't worked on this yet, since I've spent most of my time on zgw consumers upgrade for Open Zaak. Could you pick up the feedback for this PR?

@joeribekker
Copy link
Member

Refinement: Waiting for @SonnyBA to get better. If not this week, @annashamray will pick it up.

@SonnyBA SonnyBA force-pushed the issue/#197-migration-script branch from 0b18997 to 423ed1a Compare November 14, 2024 14:57
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 93.43066% with 36 lines in your changes missing coverage. Please review.

Project coverage is 96.72%. Comparing base (a8d5aa1) to head (423ed1a).
Report is 63 commits behind head on master.

Files with missing lines Patch % Lines
src/openklant/migration/v1/data.py 90.43% 11 Missing ⚠️
src/openklant/management/commands/migrate_to_v2.py 91.83% 8 Missing ⚠️
src/openklant/migration/test_server.py 38.46% 8 Missing ⚠️
src/openklant/migration/client.py 88.23% 6 Missing ⚠️
src/openklant/tests/vcr.py 85.71% 2 Missing ⚠️
src/openklant/migration/v2/data.py 96.87% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #263      +/-   ##
==========================================
- Coverage   96.89%   96.72%   -0.17%     
==========================================
  Files         146      167      +21     
  Lines        6444     7668    +1224     
==========================================
+ Hits         6244     7417    +1173     
- Misses        200      251      +51     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@SonnyBA
Copy link
Contributor Author

SonnyBA commented Nov 15, 2024

Requires #280

Edit: Since #280 was merged, this PR was rebased (see below)

@SonnyBA
Copy link
Contributor Author

SonnyBA commented Nov 15, 2024

@stevenbal I moved code from the management command file to the migration folder inside the django project dir, this makes things more readable in my opinion (separate folders for v1/v2 definitions) instead of having everything in one file. I also moved the cassettes to the migration folder inside of the project root where there also is a fixtures directory (which is used for the docker compose setup).

@SonnyBA SonnyBA requested a review from stevenbal November 15, 2024 09:44
@SonnyBA SonnyBA force-pushed the issue/#197-migration-script branch from 2ba249b to 61d0b4e Compare November 15, 2024 10:46
@alextreme
Copy link
Member

@SonnyBA please round up this PR and release a new version of Open Klant with in

Copy link
Collaborator

@stevenbal stevenbal left a comment

Choose a reason for hiding this comment

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

Some small questions/remarks, otherwise it looks good 👍

docs/development/migration.rst Outdated Show resolved Hide resolved
docs/development/migration.rst Show resolved Hide resolved
docs/development/migration.rst Outdated Show resolved Hide resolved
src/openklant/management/commands/migrate_to_v2.py Outdated Show resolved Hide resolved
src/openklant/management/commands/migrate_to_v2.py Outdated Show resolved Hide resolved
@SonnyBA SonnyBA requested a review from stevenbal November 22, 2024 10:03

An example of how one might want to run this command can be seen below:

```
Copy link
Collaborator

Choose a reason for hiding this comment

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

formatting is off for this:

image

you might have to use .. code-block:: bash

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! That fixed it:

image

@stevenbal stevenbal self-requested a review November 22, 2024 10:09
Copy link
Collaborator

@stevenbal stevenbal left a comment

Choose a reason for hiding this comment

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

Only one remark about docs formatting, otherwise good to merge for me 👍

@SonnyBA SonnyBA merged commit 60d0bfb into master Nov 22, 2024
17 checks passed
@SonnyBA SonnyBA deleted the issue/#197-migration-script branch November 22, 2024 10:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Develop OK v1 to OKv2 Klanten migration script
5 participants