Skip to content

Conversation

@Gagan-Ram
Copy link
Member

fixes: #1113

Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Sorry @Gagan-Ram, you have reached your weekly rate limit of 500000 diff characters.

Please try again later or upgrade to continue using Sourcery

@mariobehling mariobehling requested a review from Copilot October 27, 2025 21:23
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes an issue where event end dates (date_to) were not being automatically set when only a start date (date_from) was provided. The fix ensures that if an event has a start date but no end date, the end date defaults to 24 hours after the start date.

Key changes:

  • Added automatic date_to calculation in the save() method when date_to is missing but date_from exists
  • Applied the fix to two different model classes in the event.py file

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Member

@hongquan hongquan left a comment

Choose a reason for hiding this comment

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

I expect to see DB migration to change date_to to non-nullable.

@Gagan-Ram Gagan-Ram force-pushed the event_end_date branch 2 times, most recently from 16d6bf0 to ebf976a Compare October 28, 2025 17:22
@Gagan-Ram
Copy link
Member Author

This PR requires us to drop all tables of our database.

python manage.py dbshell

Then in the PostgreSQL prompt:

DROP SCHEMA public CASCADE;
CREATE SCHEMA public;
GRANT ALL ON SCHEMA public TO postgres;
GRANT ALL ON SCHEMA public TO public;

@hongquan
Copy link
Member

You should do it in one of these two ways:

  1. Accept dropping all tables (refresh our DB):
  • Just delete all migrations/xxx.py files and recreate. Every modules will starting from 0001_initial.py.
  1. Keep old data:
  • Create new migration file (don't modify 0001_initial.py).
  • Just add a step of "data migration" to the same file, where you fill a default value for date_to field of existing records (ones with null date_to), before converting the field to "non-nullable".

Your current implement is bad because it modifies an existing DB migration file.
You can modify existing DB migration files only if the changes don't cause change in DB schema (tables, columns).

The 1st option is Ok for us because we don't serve any real event yet.

@Gagan-Ram
Copy link
Member Author

3. Keep old data:

* Create new migration file (don't modify _0001_initial.py_).

* Just add a step of "[data migration](https://docs.djangoproject.com/en/5.2/topics/migrations/#data-migrations)" to the same file, where you fill a default value for `date_to` field of existing records (ones with null `date_to`), before converting the field to "non-nullable".

Thank you, this is what I was looking for.
I have made the changes, please check

@Gagan-Ram Gagan-Ram requested a review from hongquan October 30, 2025 16:24
Copy link
Member

@hongquan hongquan left a comment

Choose a reason for hiding this comment

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

@mariobehling wants to have a production system running end of this week.
We should have a fresh DB. Please delete all DB migration files and recreate from 0001. We don't have many chances to do clean up of DB migration files.


def fill_date_to(apps, _schema_editor):
Event = apps.get_model('base', 'Event')
for event in Event.objects.filter(date_to__isnull=True):
Copy link
Member

Choose a reason for hiding this comment

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

Instead of calling .save() in a for loop, you can just use Event.objects.update() with F(). It runs faster.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you

@Gagan-Ram
Copy link
Member Author

@Sak1012 We need to drop all tables of our database because of newly created migration files

python manage.py dbshell

Then in the PostgreSQL prompt:

DROP SCHEMA public CASCADE;
CREATE SCHEMA public;
GRANT ALL ON SCHEMA public TO postgres;
GRANT ALL ON SCHEMA public TO public;

@norbusan norbusan merged commit 33b917a into fossasia:enext Oct 31, 2025
1 check passed
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.

Resolve 500 error on talk dashboard

5 participants