mitodl / ocw-studio

Open Source Courseware authoring tool
BSD 3-Clause "New" or "Revised" License
7 stars 3 forks source link

added migration to remove duplicate fields and correct value #2213

Open umar8hassan opened 2 weeks ago

umar8hassan commented 2 weeks ago

What are the relevant tickets?

closes https://github.com/mitodl/hq/issues/4372

Description (What does it do?)

The migration aims to remove redundant field has_external_licence_warning and add updated field has_external_license_warning for external resources. The value however will be computed based on the external_url field of resource.

How can this be tested?

  1. Create a new course or use an existing one.
  2. Add an external resource in the course from ocw-studio.
  3. Go to admin panel and find the external resource under Website Content: http://localhost:8043/admin/websites/websitecontent/
  4. in the metadata field for the content, add has_external_licence_warning: true and save.
  5. In ocw-studio repo, switch to branch umar/4372-ocw-remove-duplicate-metadata-field-for-external-resources
  6. Apply migration: docker-compose exec web ./manage.py migrate websites
  7. Check the metadata of the resource from admin panel. It should remove has_external_licence_warning field and add or update has_external_license_warning field.
umar8hassan commented 2 days ago

The testing instructions should indicate that the migration can only be applied once without rolling back the migration (via docker compose exec web python manage.py migrate websites 0053_safedelete_deleted_by_cascade); otherwise, it's not possible to test successive changes.

The main issue is that the backwards migration is actually destructive, and will not return the database to the state that it was in previously. To reproduce this, create any external resource without the duplicate field. The metadata will contain something like has_external_license_warning. Run the migration, which will not change the WebsiteContent object. If the migration is reversed, the field in the metadata becomes has_external_licence_warning, which was not the case before the migration.

@pt2302 Yes, it won't revert DB to original state. After the fields have been removed there is no way to differentiate which object had duplicates. Reverse migration is designed to change field name to has_external_licence_warning (before the fix) for all objects as it should have been if the migration had run along with the field name fix. In reverse, all objects agree to a common field name. Which would be fixed in forward. I think, it shouldn't cause a problem.