Set registration email default to false

[dmint789]

Addresses issue #4673. This simply sets the default to registration emails for delegates and organizers to false, so that they can opt in to receiving these emails if they so choose.
Please review that I implemented the change correctly.

[dmint789]

Oops, looks like I did something wrong...

[saranshgrover]

Please update the test accordingly.

[dmint789]

I think I have fixed it. I just went with @jonatanklosko's suggestion. Haven't tested if it's working though, as I have never worked with catching emails.

[jonatanklosko]

Updating the file alone has no effect on the database, instead we need a migration. You can see a bunch of past migrations here, you can think of each migration as a script that modifies the actual database. This approach allows for applying changes to the production database, as well as the local database of every developer simply by running new migrations.

You can generate a new migration file with bin/rails g migration change_receive_registration_emails_default and usually define a change method (sometimes you may want separate up and down methods if you do less trivial things). For pretty much any kind of database structure change there's a built-in method.

Once you write the migration, you can run it locally with bin/rails db:migrate. If successful, this should apply the change to your local database and also automatically update structure.sql accordingly. There's also bin/rails db:rollback if you want to revert the migration.

When running in Docker, you need to exec these commands in the container, similarly to how you'd run tests and other stuff.

---

As for verifying the change, it's likely that some tests will fail after changing the database default, so fixing them should be a good indicator.

---

FTR: if you look at the CI build, the tests are already failing and the default is picked up, but that's because the build creates the database directly from structure.sql (since it doesn't have one already), but that's not generally the case and we need migrations.

[gregorbg]

Do you need any more guidance in writing the migration for this PR?

[dmint789]

Do you need any more guidance in writing the migration for this PR?

I'm not working on this PR at the moment. I'll try to come back to this when my schedule frees up a bit.

[dmint789]

@gregorbg a lot of time has passed since I opened this PR, so I was wondering if this issue has been dealt with elsewhere by now. If not, I'll make the change asap.
EDIT: I have made the needed change and tested that it works. If this is still relevant, please review.

[dmint789]

Please remind me how to run the migration on production. Also am I only meant to do it after this PR is merged?

[gregorbg]

The technicalities of your migration content are fine. However, you have to execute the migration locally which will change/affect the structure.sql file. The changes to that file have to be checked in as part of this PR too, otherwise the CI fails.

Migrations on prod are run manually by me or other Senior Members when deployments are done.

We should probably also announce this in an email to delegates@ because this is a substantial change in competititon workflow. Please prepare a draft in our Google Drive and reference it in a comment here before merging :)

[dmint789]

I actually have run the migration locally, but I intentionally avoided committing the changes to structure.sql, as you told me not to do this last time I made a PR with a migration. Are you sure you want me to do it this time?

[gregorbg]

For the record, Deni's question has been resolved in direct conversation. Gist: Only commit the changes to structure.sql that are actually relevant

[dmint789]

@gregorbg

We should probably also announce this in an email to delegates@ because this is a substantial change in competititon workflow. Please prepare a draft in our Google Drive and reference it in a comment here before merging :)

I have created the email draft in our Google Drive. Should I copy the text or paste a link to it?

[dmint789]

I think I have now fixed the migration

[gregorbg]

Please verify that further up in the structure.sql file, all of the actual column defaults have been changed in the individual tables' SQL CREATE TABLE definitions as well

[dmint789]

@gregorbg good catch, I've fixed it now

[dmint789]

@gregorbg just to make sure I understand correctly, the column defaults don't make a difference since the migration changes the value anyways, right? We're just doing this for the sake of elegance?

[gregorbg]

Essentially, yes. The test environment that is used to run all unit and integration tests in our CI also uses the structure.sql file to create a mock database from scratch. So keeping that file up to date helps with avoiding headaches about inconsistent behavior while testing as well :)

[dmint789]

Ugh, now the test has to be updated too, cause it's not expecting that value to be 0

[dmint789]

@gregorbg Okay, I've figured out what the issue is. We have tests that see if after a registration organizers/delegates will receive the email about it. Obviously, because the new default is false, this test fails, cause no email is sent. I can either delete these tests or change them to something else. Please let me know which course of action is better.

Fixed

[gregorbg]

Deleting tests is never a good idea as long as the basic feature they're supposed to test still exists.

In this case, the mail sending feature still exists, we just need to take a different route to trigger them. I suggest two things:

1. Adjusting the existing tests so that they create the competition with the flag set to true, and they can then expect the mail to be sent.
2. Create a second set of tests by copy/pasting (relevant parts of) the original tests and change them to expect that an email is NOT being sent. Covers both cases and gives us more confidence in our test suite.

[gregorbg]

Uh oh, wrong keybinding. I am very sorry for accidentally closing this.

Reverted changes

[dmint789]

I have looked into the tests and it looks like we actually already have tests that check that no email is sent when the checkbox is set to false, so I don't believe any new tests will be necessary. I will update the existing tests shortly.

[dmint789]

Yes! Finally got the tests to work! Please review.

[gregorbg]

Good job on figuring out the tests! LGTM but please don't merge until we can coordinate the deployment + email sendout (reach out on Slack)