Skip to content

Conversation

@tanhaow
Copy link
Contributor

@tanhaow tanhaow commented Aug 4, 2025

What's changed:

  • Removed cdhweb/blog/templates
  • Removed blog tests are also outdated
  • Removed blog tests import from cdhweb/conftest.py
  • Skipped the unit test related to blog posts in cdhweb/people/tests/test_pages.py

@tanhaow tanhaow self-assigned this Aug 4, 2025
@tanhaow tanhaow requested a review from rlskoeser August 5, 2025 13:41
Copy link
Contributor

@rlskoeser rlskoeser left a comment

Choose a reason for hiding this comment

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

I thought cleaning up the templates you flagged as unused would be simpler than this, but it feels like we're pulling on a thread and I can't tell if it's going to start unraveling the sweater or it's part of an unused clump of yarn. It might be better to leave as is.

I do have a niggling memory that the Springload team kept around old versions of some of the models and templates around for the migration and I'm not sure if they ever cleaned up the code after the migration was complete.

Let's put this on hold and I'll check with Carrie and Springload folks.

person.delete()
assert Profile.objects.count() == 0

@pytest.mark.skip("Skipped after blog tests cleanup - blog integration test")
Copy link
Contributor

Choose a reason for hiding this comment

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

I am suspicious of this change... The test should either be removed or fixed, not skipped; you've removed a fixture used for the test, so it would never work again the way it did previously. (And would be harder to figure out later when the fixture is gone but no the test.)

However, I am puzzled why it would be working but not impacted by the code you've removed. I looked briefly at the test fixture and I think maybe it's old code that's no longer used by the running application - but that feels like a bigger change than we intended.

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.

3 participants