Skip to content
This repository has been archived by the owner on Nov 21, 2024. It is now read-only.

Add verification page before unregistering from event from email #357 #408

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -66,3 +66,6 @@ config.yaml
amivapi/config.yaml
apikeys.yaml
amivapi_storage

# Pycharm
.idea/*
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: missing newline.

44 changes: 25 additions & 19 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ You only need to install Docker, nothing else is required.

### Manual Installation for Development

For development, we recommend to clone the repository and install AMIV API
For development, we recommend to fork the repository and install AMIV API
manually.

First of all, we advise using a [virtual environment](https://docs.python.org/3/tutorial/venv.html).
Expand Down Expand Up @@ -72,8 +72,13 @@ The following command runs a MongoDB service available on the default port
password `amivapi`.

```sh
# Initialize "swarm", a scheduling and clustering tool, that will enable us to create a network overlay
docker swarm init

# Create a network so that the api service can later be connected to the db
docker network create --driver overlay backend

#
Copy link
Contributor

Choose a reason for hiding this comment

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

This line seems to serve no purpose?

docker service create \
--name mongodb -p 27017:27017 --network backend\
-e MONGODB_DATABASE=amivapi \
Expand Down Expand Up @@ -102,7 +107,7 @@ Now it's time to configure AMIV API. Create a file `config.py`
ROOT_PASSWORD = 'root'

# MongoDB Configuration
MONGO_HOST = 'mongodb'
MONGO_HOST = 'mongodb' # or 'localhost' if you run the database locally
MONGO_PORT = 27017
MONGO_DBNAME = 'amivapi'
MONGO_USERNAME = 'amivapi'
Expand Down Expand Up @@ -152,7 +157,7 @@ Configuration files can be used easily for services using
docker config create amivapi_config config.py
```

Now start the API service (make sure to put it in the same network as MongoDB
Now start the API service (make sure to put it in the same network (here `backend`) as MongoDB
if you are running a MongoDB service locally).

```sh
Expand All @@ -169,30 +174,31 @@ docker service create \
--name amivapi-cron --network backend \
--config source=amivapi_config,target=/api/config.py \
amiveth/amivapi amivapi cron --continuous

# To attach your command line to the docker instance, use
docker attach amivapi... # Use tab completion to find the name of the service
Copy link
Contributor

Choose a reason for hiding this comment

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

Thinking about it: This is not at all how one should use a docker service, and not really required for the amivapi, so I don't think we should recommend this here.


# As we run docker as a service, it restarts by itself even if you use docker kill
# To stop the service, use
docker service rm amivapi
```

(If you want to mount the config somewhere else, you can use the environment
variable `AMIVAPI_CONFIG` to specify the config path in the container.)

### Run locally

If you have installed AMIV API locally, you can use the CLI to start it:
If you have installed AMIV API locally, you can use the CLI to start it.

```sh
# Start development server
amivapi run dev
First, change `MONGO_HOST = 'mongodb'` in `config.py` to `'MONGO_HOST = 'localhost'`.
Then, in CLI (with the environment active):

# Start production server (requires the `bjoern` package)
amivapi run prod

# Execute scheduled tasks periodically
amivapi cron --continuous

# Specify config if its not `config.py` in the current directory
amivapi --config <path> run dev

# Get help, works for sub-commands as well
amivapi --help
```sh
amivapi run dev # Start development server
amivapi run prod # Start production server (requires the `bjoern` package)
amivapi cron --continuous # Execute scheduled tasks periodically
amivapi --config <path> run dev # Specify config if its not `config.py` in the current directory
amivapi --help # Get help, works for sub-commands as well
amivapi run --help
```

Expand All @@ -204,7 +210,7 @@ amivapi run --help
If you have docker installed you can simply run the tests in a Docker instance:

```sh
./run_tests.sh
./run_tests.sh # potentially try with sudo
```

By default, this will start a container with mongodb, and run
Expand Down
54 changes: 51 additions & 3 deletions amivapi/events/email_links.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,16 +7,18 @@
Needed when external users want to sign up for public events or users want to
sign off via links.
"""
from datetime import datetime
from bson import ObjectId
from eve.methods.delete import deleteitem_internal
from eve.methods.patch import patch_internal
from flask import Blueprint, current_app, redirect
from flask import Blueprint, current_app, redirect, make_response,\
render_template
from itsdangerous import BadSignature, URLSafeSerializer

from amivapi.events.queue import update_waiting_list
from amivapi.events.utils import get_token_secret

email_blueprint = Blueprint('emails', __name__)
email_blueprint = Blueprint('emails', __name__, template_folder='templates')


def add_confirmed_before_insert(items):
Expand Down Expand Up @@ -66,12 +68,58 @@ def on_delete_signup(token):
try:
s = URLSafeSerializer(get_token_secret())
signup_id = ObjectId(s.loads(token))

except BadSignature:
return "Unknown token"

# Verify if user confirmed
# definitive = request.args.get('DEFINITIVE_DELETE')
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this comment serve any purpose?

# Get first name for personal greeting
error_msg = ''
query = {'_id': signup_id}
data_signup = current_app.data.driver.db['eventsignups'].find_one(query)
if data_signup is None:
error_msg = "This event might not exist anymore or the link is broken."
user = data_signup['user']
if user is None:
user = data_signup['email']
else:
query = {'_id': user}
data_user = current_app.data.driver.db['users'].find_one(query)
user = data_user["firstname"]
event = data_signup['event']
query = {'_id': event}
data_event = current_app.data.driver.db['events'].find_one(query)
event_name = data_event["title_en"]
if event_name is None:
event_name = data_event["title_en"]
if data_event["time_start"] is None:
event_date = "a yet undefined day."
else:
event_date = datetime.strftime(data_event["time_start"],
'%Y-%m-%d %H:%M')
# Serve the unregister_event page
response = make_response(render_template("unregister_event.html",
user=user,
event=event_name,
event_date=event_date,
error_msg=error_msg,
token=token))
response.set_cookie('token', token)
return response


@email_blueprint.route('/delete_confirmed/<token>', methods=['POST'])
def on_delete_confirmed(token):
try:
s = URLSafeSerializer(get_token_secret())
signup_id = ObjectId(s.loads(token))

except BadSignature:
return "Unknown token"

deleteitem_internal('eventsignups', concurrency_check=False,
**{current_app.config['ID_FIELD']: signup_id})

redirect_url = current_app.config.get('SIGNUP_DELETED_REDIRECT')
if redirect_url:
return redirect(redirect_url)
Expand Down
2 changes: 0 additions & 2 deletions amivapi/events/model.py
Original file line number Diff line number Diff line change
Expand Up @@ -257,7 +257,6 @@
'description': 'Start time of the event itself. If you define '
'a start time, an end time is required, too.',
'example': '2018-10-17T18:00:00Z',

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it necessary to remove this newline? I think it helped the readability.

Copy link
Author

Choose a reason for hiding this comment

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

Ah... for me it is the contrary, it is disturbing, as it should be part of the same json structure on the same level, I thought that it was clearer to keep it on one level.

Copy link
Contributor

Choose a reason for hiding this comment

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

In this case I would add the line again, as this has nothing to do with the feature of this PR.

'type': 'datetime',
'nullable': True,
'default': None,
Expand All @@ -269,7 +268,6 @@
'description': 'End time of the event itself. If you define '
'an end time, a start time is required, too.',
'example': '2018-10-17T22:00:00Z',

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it necessary to remove this newline? I think it helped the readability.

'type': 'datetime',
'nullable': True,
'default': None,
Expand Down
47 changes: 47 additions & 0 deletions amivapi/events/templates/unregister_event.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
<html lang="en">
<head>
<meta charset="utf-8" />
<meta name="viewport" content="width=device-width, initial-scale=1" />
<title>AMIV Unregister from Event</title>
<style type="text/css">
{% include "style.css" %}
</style>
</head>
<body>
{% if error_msg %}
<dialog open>
<p>{{ error_msg }}</p>
<button onclick="this.parentNode.close()">close</button>
</dialog>
{% endif %}

<aside>
<img src={{ url_for('static', filename='logo.png') }} alt="Logo">
</aside>

<main>
<h1>
{% if user %}
Hi {{user}}!
{% else %}
Hello!
{% endif %}
</h1>
<p>
{% if user and event %}
We will irrevocably unregister you (<em>{{user}}</em>) from the event <em>{{event}}</em> on {{event_date}}.
Is that ok?
{% else %}
You clicked on a opt-out link of the AMIV at ETHZ student organization. We cannot process your request,
because we either do <em>not</em> know the event you wish to unregister from, or your user name, or both.
{% endif %}
</p>

<form action="/delete_confirmed/{{token}}" method="POST">
<input type="submit" value="I agree">
</form>

</main>

</body>
</html>
3 changes: 3 additions & 0 deletions amivapi/settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,9 @@
REMOTE_MAILING_LIST_ADDRESS = None
REMOTE_MAILING_LIST_KEYFILE = None
REMOTE_MAILING_LIST_DIR = './' # Use home directory on remote by default
# Signups via email (@email_blueprint.route('/delete_signup/<token>')
# in email_links.py)
# DEFINITIVE_DELETE = ''
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this commented out? Is it needed?


# SMTP server defaults
API_MAIL = '[email protected]'
Expand Down
6 changes: 5 additions & 1 deletion amivapi/tests/events/test_emails.py
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,9 @@ def test_email_signup_delete(self):

# With redirect set
self.app.config['SIGNUP_DELETED_REDIRECT'] = "somewhere"
self.api.get('/delete_signup/%s' % token, status_code=302)
self.api.get('/eventsignups/%s' % signup['_id'], status_code=200)
self.api.get('/delete_signup/%s' % token, status_code=200)
self.api.post('/delete_confirmed/%s' % token, status_code=302)
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for adding tests ... unfortunately I do not understand at all what is going on here, and why e.g. in this last line a status code 302 is expected.

As the flow is now considerably more complicated, please either:

  • Split this test up into multiple tests that are clear about which part is being tested.
  • If it is impossible to split them up, please modify the docstring to explain the expected behavior.

The more I look at it, the more I think our original tests here are already confusing. I would greatly appreciate it if you could try to split the tests up a bit more to make them more understandable.


# Check that signup was deleted
self.api.get('/eventsignups/%s' % signup['_id'], status_code=404)
Expand All @@ -72,7 +74,9 @@ def test_email_signup_delete(self):

# Without redirect set
self.app.config.pop('SIGNUP_DELETED_REDIRECT')
self.api.get('/eventsignups/%s' % signup['_id'], status_code=200)
self.api.get('/delete_signup/%s' % token, status_code=200)
self.api.post('/delete_confirmed/%s' % token, status_code=200)

# Check that signup was deleted
self.api.get('/eventsignups/%s' % signup['_id'], status_code=404)
Expand Down