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

Add default alias name to custom domain #154

Merged
merged 1 commit into from
May 3, 2020

Conversation

SibrenVasse
Copy link
Contributor

@SibrenVasse SibrenVasse commented May 3, 2020

Implementation for https://trello.com/c/dz9npgnw/66-able-to-set-alias-name-for-a-domain

The name in the from header is first set by the alias name, then the default alias name from the domain and otherwise not present.

Implementation runtime tested. I'm not sure if I did the migration correctly, as I could not find any documentation on this. I did my best to try and follow a similar style to the rest of the codebase. Let me know what you think.

Copy link
Contributor

@nguyenkims nguyenkims left a comment

Choose a reason for hiding this comment

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

Overall the PR is perfect! I just left some rather nitpicking comments :).

app/dashboard/views/domain_detail.py Outdated Show resolved Hide resolved
email_handler.py Outdated
if alias.name:
LOG.d("Put alias name in from header")
from_header = formataddr((alias.name, alias.email))
else:
from_header = alias.email
elif alias_domain not in ALIAS_DOMAINS:
Copy link
Contributor

Choose a reason for hiding this comment

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

We can use alias.custom_domain_id here I think. This field is filled up if the alias is dynamically created thanks to the directory feature.

We can also create a relationship like custom_domain = db.relationship("CustomDomain", foreign_keys=[custom_domain_id]) to avoid doing a CustomDomain.get(), this is the same in terms of number of query to the database though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I ended up adding the relationship in the model, this enabled me to display the domain-wide alias name as a placeholder in the alias view.

This does make alias.custom_domain_id redundant, but it has quite a few users left.

@nguyenkims
Copy link
Contributor

@SibrenVasse Thanks a lot for the PR!

For the database migration, this requires having a Postgres database indeed. On local environment, a sqlite database is created by the db.create_all() that bypasses the migration. This is convenient for development and unit tests as we don't have to wait for the migration.

If you have docker, you can create the migration by the following command: it first creates a postgres database for SimpleLogin, then run flask db upgrade to upgrade the DB to the latest stage and finally flask db migrate to generate the migration script. Let me know if this makes sense :).

docker rm -f sl-db
docker run -p 5432:5432 --name sl-db -e POSTGRES_PASSWORD=postgres -e POSTGRES_DB=sl -d postgres

env DB_URI=postgresql://postgres:postgres@127.0.0.1:5432/sl flask db upgrade

env DB_URI=postgresql://postgres:postgres@127.0.0.1:5432/sl flask db migrate

docker rm -f sl-db

@SibrenVasse
Copy link
Contributor Author

The migration (up and down) ran successfully with your instructions.

email_handler.py Outdated Show resolved Hide resolved
@SibrenVasse
Copy link
Contributor Author

I've resolved your comments.

@nguyenkims
Copy link
Contributor

@SibrenVasse perfect, thanks a lot! We'll test on staging and then deploy onto production today or tomorrow.

@nguyenkims nguyenkims merged commit 042a421 into simple-login:master May 3, 2020
@nguyenkims nguyenkims mentioned this pull request May 4, 2020
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.

None yet

2 participants