Skip to content

Small improvements for displaying closed funds#4456

Merged
frjo merged 4 commits intomainfrom
enhancement/improvements-for-closed-funds
Mar 19, 2025
Merged

Small improvements for displaying closed funds#4456
frjo merged 4 commits intomainfrom
enhancement/improvements-for-closed-funds

Conversation

@wes-otf
Copy link
Contributor

@wes-otf wes-otf commented Mar 18, 2025

A couple simple additions:

  • Added a new message (as a customizable system setting) that will display when there are no open funds
  • Added call to action to closed round message along with a fix for displaying the verbose name
  • Small misc. fixes/additions (Mobil -> Mobile typo in system settings, docstrings in util_tags.py

Test Steps

  • Ensure that the set system settings message displays on the home page when no funds are live
  • Ensure there is a link to the homepage in the message displayed when a closed round URL is accessed

@wes-otf wes-otf added the Type: Enhancement This is an improvement of an existing thing (not a new thing, which would be a feature). label Mar 18, 2025
@wes-otf wes-otf requested a review from frjo March 18, 2025 20:29
@wes-otf wes-otf force-pushed the enhancement/improvements-for-closed-funds branch from f5480e3 to d034879 Compare March 18, 2025 21:10
@wes-otf
Copy link
Contributor Author

wes-otf commented Mar 18, 2025

this test failure is super weird - no errors when running the server & it pulls the message from the system setting fine? But tests all error out saying the column doesn't exist. will dig deeper tomorrow

@frjo
Copy link
Member

frjo commented Mar 19, 2025

@theskumar Can you figure out why the test fails like this? It's like the migration does not run before the tests.

The code looks fine to me, just another home page setting. The site works as intended with this new setting.

Copy link
Member

@frjo frjo left a comment

Choose a reason for hiding this comment

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

Something weird with the tests. The code works just as intended when I test.

@theskumar
Copy link
Member

@theskumar Can you figure out why the test fails like this? It's like the migration does not run before the tests.

The code looks fine to me, just another home page setting. The site works as intended with this new setting.

Checking

{% else %}
<div class="mt-8">
{{ settings.core.SystemSettings.home_no_applications_msg|nh3|safe }}
<div>
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be a closing div </div>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nice catch! not sure why I'm forgetting to properly close all my tags

@wes-otf wes-otf force-pushed the enhancement/improvements-for-closed-funds branch from 235b0e6 to ed9a2d1 Compare March 19, 2025 14:46
@wes-otf
Copy link
Contributor Author

wes-otf commented Mar 19, 2025

This testing failure is so weird - seems to happen when any new field gets added in a migration. Like I added a test attribute to the Activity model on a clean branch:

class Activity(models.Model):
    test = models.TextField(default="test")
    ...

which resulted in the same django.db.utils.ProgrammingError: column activity_activity.test does not exist despite migrations running fine on my local DB. can't find out how to debug pytest-django's migration running to see where/why it's not running if that's the case, maybe a previous migration is messing with things?

@theskumar
Copy link
Member

This testing failure is so weird - seems to happen when any new field gets added in a migration. Like I added a test attribute to the Activity model on a clean branch:

class Activity(models.Model):
    test = models.TextField(default="test")
    ...

which resulted in the same django.db.utils.ProgrammingError: column activity_activity.test does not exist despite migrations running fine on my local DB. can't find out how to debug pytest-django's migration running to see where/why it's not running if that's the case, maybe a previous migration is messing with things?

By default, pytest-django creates a test database the first time a test requires it. This database is cached for subsequent tests, which helps improve performance by avoiding repeated setup, unless you provide --reuse-db parameter.

@theskumar theskumar force-pushed the enhancement/improvements-for-closed-funds branch from b29f849 to ed9a2d1 Compare March 19, 2025 18:55
@wes-otf
Copy link
Contributor Author

wes-otf commented Mar 19, 2025

This database is cached for subsequent tests, which helps improve performance by avoiding repeated setup, unless you provide --reuse-db parameter

@theskumar did you mean --create-db? I was trying to get it remade each time just to see if that would change anything but was using create, maybe I should try with reuse if that's the case

@theskumar
Copy link
Member

theskumar commented Mar 19, 2025

The issue is that in the previous migration core -> 0007, SystemSettings is loaded. The Python version of the class in the codebase expects the home_no_applications_msg to be present, but this property is not available until the migration 0008 is run.

image

To reproduce, create a fresh database, update your DATABASE_URL, and try running ./manage.py migrate.

@theskumar theskumar mentioned this pull request Mar 19, 2025
theskumar and others added 2 commits March 20, 2025 01:11
How to test. 

- create a fresh db
- update you DATABASE_URL
- run `./manage.py migrate

issue:
#4456 (comment)

Strategy: 
- fetch only the required columns, and update data only if previous
system setting exist.
@wes-otf
Copy link
Contributor Author

wes-otf commented Mar 19, 2025

thanks @theskumar for saving me quite a bit of time! I was stumped

@frjo frjo added the Type: Patch Mini change, used in release drafter label Mar 19, 2025
@frjo frjo merged commit 5622eda into main Mar 19, 2025
7 checks passed
@theskumar theskumar deleted the enhancement/improvements-for-closed-funds branch March 20, 2025 03:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Type: Enhancement This is an improvement of an existing thing (not a new thing, which would be a feature). Type: Patch Mini change, used in release drafter

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants