@W-14603959 - Update backend deps and test failure fixes #2154
Conversation
There was a problem hiding this comment.
Thanks for slogging through these. Some changes requested, including sticking to Django 4.2.x. (django-allauth to 0.59.0)
metecho/api/models.py
Outdated
| def save(self, *args, **kwargs): | ||
| obj = super().save(*args, **kwargs) | ||
| self.update_status() | ||
| return super().save(*args, **kwargs) | ||
| return obj | ||
|
|
There was a problem hiding this comment.
Django 4.1 introduces a change where using reverse foreign keys before saving an object raises a ValueError. Since the update_status method interacts with related objects, as seen in methods like should_update_review, we have issues with reverse foreign key lookups.
You're correct that calling super().save() to ensure the Epic instance has an ID before update_status is the right fix. But the way it's written now, the status gets updated but not saved in the database.
def save(self, *args, **kwargs):
if not self.id:
super().save(*args, **kwargs)
self.update_status()
return super().save()This code makes sure that if the Epic is new, we save it before updating the status. By not repeating *args, **kwargs in the final save, we avoid duplicating related objects.
requirements/prod.txt
Outdated
| # via -r requirements/prod.in | ||
| django==4.0.6 | ||
| django==5.0 |
There was a problem hiding this comment.
Let's stick to the LTS for now. You can restrict which version of Django we use in prod.in:
Django~=4.2.0 # LTS
(Where ~= means compatible release.)
| @pytest.fixture | ||
| def dummy(): | ||
| app = SocialApp.objects.create( | ||
| provider=CustomSalesforceProvider.id, | ||
| name=CustomSalesforceProvider.id, | ||
| client_id="app123id", | ||
| key=CustomSalesforceProvider.id, | ||
| secret="dummy",) | ||
| return app | ||
|
|
||
|
|
||
| @pytest.mark.django_db | ||
| def test_get_auth_params(rf, dummy): | ||
| request = rf.get("/") | ||
| result = CustomSalesforceProvider(request).get_auth_params(request, None) | ||
| result = CustomSalesforceProvider(request, dummy).get_auth_params(request, None) | ||
| assert "prompt" in result and result["prompt"] == "login" | ||
|
|
||
|
|
||
| @pytest.mark.django_db | ||
| def test_extract_uid(rf): | ||
| request = rf.get("/") | ||
| provider = CustomSalesforceProvider(request) | ||
| provider = CustomSalesforceProvider(request, dummy) | ||
| result = provider.extract_uid({"organization_id": "ORG", "user_id": "USER"}) | ||
| assert result == "ORG/USER" |
There was a problem hiding this comment.
My mistake. Let's just use the factory_boy fixture instead:
@pytest.mark.django_db
def test_get_auth_params(rf, social_app_factory):
request = rf.get("/")
app = social_app_factory(
name=CustomSalesforceProvider.id,
provider=CustomSalesforceProvider.id,
)
result = CustomSalesforceProvider(request, app).get_auth_params(request, None)
assert "prompt" in result and result["prompt"] == "login"
metecho/api/tests/admin.py
Outdated
| @@ -42,7 +42,7 @@ def test_queryset__deleted(self, epic_factory): | |||
|
|
|||
| args = ( | |||
| None, | |||
| {"deleted_at": "true"}, | |||
| {"deleted_at": ["true"]}, | |||
There was a problem hiding this comment.
We can set this aside until we do move to Django 5x:
| {"deleted_at": ["true"]}, | |
| {"deleted_at": "true"}, |
|
@jstvz Thanks for the detailed review. Addressed the review comments and got to know the usage of factory_boy now. |
This reverts commit c12b902.
No description provided.