migration mot working with existing data

[wingechr]

https://github.com/OpenEnergyPlatform/oeplatform/blob/f7b89217cb27ab8dcee70b6a6a92986b221b9f61/dataedit/migrations/0034_alter_peerreview_oemetadata.py#L16

@jh-RLI removing null=True without default does not work with existing data

[jh-RLI]

Yes! Urg i fixed that in one of the active branches and then forgot about it. Sorry!

Related: In some migrations during Development fields are added and removed again. It would be Good to clean the migrations up before we merging.

[wingechr]

just to make sure that the next rollout will run smoothly: strangely enough, the productive oep django is further down then the test toep:

oep> python manage.py showmigrations

...
 [X] 0020_peerreview
 [X] 0021_rename_in_progress_peerreview_is_finished
 [X] 0022_peerreview_datestarted
 [X] 0023_auto_20230414_0032
 [X] 0024_alter_peerreview_is_finished
 [X] 0025_auto_20230427_1403
 [X] 0026_alter_peerreviewmanager_is_open_since
 [X] 0027_alter_peerreviewmanager_is_open_since
 [X] 0020_alter_table_unique_together
 [X] 0028_merge_20230511_1516
 [X] 0029_alter_peerreviewmanager_current_reviewer
 [X] 0030_peerreview_is_publish
 [X] 0031_auto_20231204_1451
 [X] 0032_alter_table_is_publish
 [X] 0033_peerreview_oemetadata
 [ ] 0034_alter_peerreview_oemetadata
...

toep> python manage.py showmigrations

...
 [X] 0020_peerreview
 [X] 0021_rename_in_progress_peerreview_is_finished
 [X] 0022_peerreview_datestarted
 [X] 0023_auto_20230414_0032
 [X] 0024_alter_peerreview_is_finished
 [X] 0025_auto_20230427_1403
 [X] 0026_alter_peerreviewmanager_is_open_since
 [X] 0027_alter_peerreviewmanager_is_open_since
 [X] 0020_alter_table_unique_together
 [X] 0028_merge_20230511_1516
 [X] 0029_alter_peerreviewmanager_current_reviewer
 [X] 0030_peerreview_is_publish
 [X] 0031_auto_20231204_1451
 [X] 0032_alter_table_is_publish
...

[jh-RLI]

This is due to a "dirty" release. Too bad we had to do this, but at least this is the first one in about a half year. Thanks for documenting this again. I've already mentioned that it's fixed in a branch (which is still in draft), but I'll create a separate branch today and put the fix in there so we can have it in the next release.

[jh-RLI]

I assume that the difference OEP-TOEP is only due to the field that is nullable and existing/not existing data for this fields.

[wingechr]

hopefully, but also: the migrations 0033 and 0034 seem not to exist on toep

[jh-RLI]

Yes, fortunately we now have access so that we can carry out the releases according to the release procedure (1. toep 2. oep ;) ) At that point we only updated the oep due to time limitations .... :/

[jh-RLI]

I think we should also delete all reviews again. There is the management command manage.py clear_peer_reviews --all | "id" AS we discussed in the Project meeting this feature is still not complete and will either be deactivated until it is or moved to a less popular section on the website and marked as "early access" feature. Because it is not complete, we can delete all peer reviews IMO but let's first check if there are any relevant ones because it is also possible to save them and apply them later once the feature is complete.

[jh-RLI]

EDIT: I think I can fix this :) so don't waste your precious time :)

@wingechr I have added a test for the migration - commit below. I don't think I'm doing it right. My problem is that I need to create a user and a table and then link the FKs in the test entry in the peerreview table in the database. I think I should use a function from the view that would "normally" create the review entry, but the view is very bloated so this logic is part of the PeerReviewView get method.

Do you think setting the database up manually (like i attempt in the test) is a good idea? I think i really should clean the PeerReview view ups and make them more modular. Also i think the

[wingechr]

mh,I don't have any experience with these kind of migration tests. I worry it will not work further down the line, even if it works now.

Test always will be run after all migrations have completed. So in a later stage of development, this will probably break the test.

[jh-RLI]

Okay, hmm I think the test now is kind of isolated but also makes less sense now because it's not running the migration but is only testing if the function is running (that is used in the migration). If that test fails in the future, that should only be the case if I do away with the peerReview model. That seems to be feasible.

In genreal as I said, for now it is ok to delete all existing peer reviews, this should be done before applying this migration. Later the migration will be applied as expected and I assume that there will be no more incomplete data. I think if the issue with existing data is no longer a problem then this migration should also be okay? or am i missisng something.

To propperly test migrations i assume we should use this approach or use the django-test-migrations package,