Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Let users choose which emails to receive #369

Merged
merged 1 commit into from
Jul 18, 2021

Conversation

HebaruSan
Copy link
Contributor

@HebaruSan HebaruSan commented Jun 2, 2021

Motivation

Currently if you follow a mod, you receive all email notifications for it. Some users might not want to receive notifications for compatibility updates.

Changes

Fixes #333 (the part about ignoring the auto-update email).

UI

Now if you edit your profile, a new section lists the mods you follow, with two checkboxes for each. The rows are striped to make it easier to pick out the associations between the contents of each row:

image

If you uncheck a checkbox (and save your changes), you won't receive that notification for that mod anymore. Each column of checkboxes has "Check all" and "Uncheck all" buttons that check and uncheck all the checkboxes in that column, so users can easily disable all notifications of a particular type.

Admins are allowed to edit these settings for other users in case someone requests it and can't figure out how to do it.

Data model

To make this possible, the mod_followers table has two new bool columns, both set to True for all old rows:

  • send_update - True to send regular update notifications, False to suppress them
  • send_autoupdate - True to send auto-update notifications, False to suppress them

In addition, mod_followers now has three new indices:

  • mod_id to make it faster to find all of the followers of a mod
  • user_id to make it faster to find all the mods a user follows
  • (user_id, mod_id) to make it faster to find a particular user's follow row for a particular mod

Side fixes

The user description field in the profile edit screen now uses the rich text editor; the field has always supported markdown, but the rich text editor wasn't enabled before.

@HebaruSan HebaruSan added Area: Backend Related to the Python code that runs inside gunicorn Area: Email Related to automated emails that we send to users Priority: Normal Type: Feature Status: Ready Area: Frontend Related to HTML, JS, CSS, or other browser things Area: Migration Related to Alembic database migrations labels Jun 2, 2021
@HebaruSan HebaruSan requested a review from DasSkelett June 2, 2021 16:39
@HebaruSan HebaruSan force-pushed the feature/email-settings branch 2 times, most recently from cf73bb6 to fea3ff4 Compare June 2, 2021 18:46
@HebaruSan

This comment has been minimized.

@DasSkelett

This comment has been minimized.

@HebaruSan HebaruSan force-pushed the feature/email-settings branch 2 times, most recently from 1d76bb9 to 0e36cde Compare July 2, 2021 16:07
@HebaruSan
Copy link
Contributor Author

HebaruSan commented Jul 2, 2021

Ahh right, I probably forgot to check out and pull alpha. Should be OK now?
(I guess I should make a habit of rebasing on upstream/alpha instead of just alpha.)

@DasSkelett
Copy link
Member

The migration and code in objects.py don't match currently, and there are also issues with the relationsships between User<->Following<->Mod.
Attempting a fix here: HebaruSan#3

@HebaruSan
Copy link
Contributor Author

The migration and code in objects.py don't match currently, and there are also issues with the relationsships between User<->Following<->Mod.

How does this look after merging your fixes?

@DasSkelett
Copy link
Member

How does this look after merging your fixes?

We should be fine now, alembic generates empty revisions. The indexes are used as well, after filling the tables with some data, so I'm confident they're used on production as well.

@DasSkelett
Copy link
Member

We should also specify some col-xs-* classes for the email settings container, with the "default" layout it's hard to recognize which "Check all" / "Uncheck all" buttons toggle which option on smartphones:

Screenshot

image

I've managed to split them into two columns again, so it's easier to understand which buttons are for which option.

Screenshot

image

Do you like this layout? If so, I can push a commit. It's not much, just adding col-xs-12 to the columns with col-md-3, and col-xs-6 to col-md-2.

@HebaruSan
Copy link
Contributor Author

Do you like this layout?

Yes, that looks a lot better, thanks!

@DasSkelett
Copy link
Member

One more small bug I just found in testing, in the auto-update email subject we take the game version of mod.versions[0], while it's mod.default_version that is being updated. usually it's the same, but not necessarily.
In the email body the correct one is used already.
Fixed in the latest commit.

Copy link
Member

@DasSkelett DasSkelett left a comment

Choose a reason for hiding this comment

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

Looks solid to me. If you want, you can squash before merge.

@HebaruSan HebaruSan merged commit 7186839 into KSP-SpaceDock:alpha Jul 18, 2021
@HebaruSan HebaruSan deleted the feature/email-settings branch July 18, 2021 18:07
@HebaruSan
Copy link
Contributor Author

Took me a while to figure out how to run migrations on alpha. For future reference, this seemed to work:

cd /SpaceDock
bin/alembic upgrade 17fbd4ff8193

@DasSkelett
Copy link
Member

DasSkelett commented Jul 18, 2021

Sorry, forgot about it this time. bin/alembic upgrade head works as well.

@HebaruSan
Copy link
Contributor Author

No problem, it's something I should learn anyway. I'll try to remember the head alias is available.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Backend Related to the Python code that runs inside gunicorn Area: Email Related to automated emails that we send to users Area: Frontend Related to HTML, JS, CSS, or other browser things Area: Migration Related to Alembic database migrations Priority: Normal Status: Ready Type: Feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants