Add verification page before unregistering from event from email #357 by lioneltrebuchon · Pull Request #408 · amiv-eth/amivapi

lioneltrebuchon · 2019-11-23T21:04:26Z

No description provided.

…ing list

…formation

NotSpecial · 2019-11-23T21:34:42Z

Can you maybe provide a screenshot how this unregister page looks like? Does it work with different screen formats?

NotSpecial · 2019-11-23T21:35:43Z

And additionally, could you provide some reasoning about why we need this functionality in the api?

NotSpecial

Thank you for your work!

Could you please:

Provide some reasoning/background on why this feature is required and implemented like this.
Address the comments in this review.
Implement tests for the new functionality.

Thank you!

NotSpecial · 2019-11-23T21:36:28Z

.gitignore

 amivapi_storage
+
+# Pycharm
+.idea/*


Nitpick: missing newline.

README.md

amivapi/settings.py

amivapi/utils.py

lioneltrebuchon · 2019-11-23T22:09:57Z

Implementation of two stage deletion as per
#357

NotSpecial

Thank you for your comments. I have some more suggestions.

On a different note: The changes to README.md seem quite unrelated to this PR. Maybe it would be better to open a separate PR for this, to only keep the relevant changes in this one?

NotSpecial · 2019-11-24T16:32:25Z

README.md


 # MongoDB Configuration
-MONGO_HOST = 'mongodb'
+MONGO_HOST = 'mongodb' # or 'localhost' if you run the API from Python


Suggested change

MONGO_HOST = 'mongodb' # or 'localhost' if you run the API from Python

MONGO_HOST = 'mongodb' # or 'localhost' if you run the database locally

NotSpecial · 2019-11-24T16:34:10Z

amivapi/events/model.py

                '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',
-


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

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.

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

NotSpecial · 2019-11-24T16:34:14Z

amivapi/events/model.py

                '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',
-


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

NotSpecial · 2019-11-24T16:35:07Z

amivapi/events/templates/unregister_event.html

+        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.


Whats up with the curly braces around the not? Is this some special html syntax I don't know?

NotSpecial · 2019-11-24T16:36:00Z

amivapi/settings.py

 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 = ''


Why is this commented out? Is it needed?

NotSpecial · 2019-11-24T16:39:27Z

amivapi/settings.py

 API_MAIL = 'api@amiv.ethz.ch'
 API_MAIL_SUBJECT = "[AMIV] {subject}"
-SMTP_HOST = 'localhost'
+SMTP_HOST = 'localhost' # None in case you want to accept that no mails get sent (local testing)


Thanks, I understand the comment now. But what exactly is the problem with leaving it on localhost for local testing? Is there some error?

I removed it because I haven't been able to reproduce any error.

NotSpecial · 2019-11-24T16:41:58Z

Note: I have updated this branch to the new master branch, make sure to pull before working on it again.

When users are upgraded to members via LDAP, subscribe them to the newsletter. Newsletter settings of already imported members are not touched. Closes amiv-eth#400

- Add documentation for position field. - Add position field to post response. Fixes amiv-eth#405 Fixes amiv-eth#410

Even if FCFS in enabled, admins should be allowed to force a signup to be accepted. This worked as intended for PATCH requests already, and now also works for POST. Fixes amiv-eth#381

lioneltrebuchon · 2019-12-01T17:30:34Z

@temparus I believe the base is working. Haven't changed much.

NotSpecial

Thank you for your continued work!

Most of all, thank you for adding tests. However, I am currently having a hard time to understand what is being tested, and what the expected functionality is, so I can't really judge whether the tests are sufficient or not.

Please try to:

break the tests up a little,
explain the expected behavior more.

This would be greatly appreciated!

I have a few nitpicks here and there as well, please try to address them, too.

NotSpecial · 2019-12-01T18:09:22Z

README.md

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


This line seems to serve no purpose?

NotSpecial · 2019-12-01T18:10:37Z

README.md

    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


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.

NotSpecial · 2019-12-01T18:11:19Z

amivapi/events/email_links.py

+        return "Unknown token"
+
+    # Verify if user confirmed
+    # definitive = request.args.get('DEFINITIVE_DELETE')


Does this comment serve any purpose?

NotSpecial · 2019-12-01T18:13:10Z

amivapi/events/model.py

                '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',
-


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

NotSpecial · 2019-12-01T18:18:17Z

amivapi/tests/events/test_emails.py

-        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)


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.

lioneltrebuchon added 5 commits November 23, 2019 01:33

Enhance documentation, take note about unregistering from events mail…

d8dae95

…ing list

Enhance README.md with how to run API locally (Python)

4d84d06

WIP on adding one security to unregister from event

cb1ec32

Transmit token to unregister page

1bf724c

Finalize unregister function, force POST request and includes date in…

12e674f

…formation

NotSpecial suggested changes Nov 23, 2019

View reviewed changes

Remove typos and printouts

f52e0bc

lioneltrebuchon changed the title ~~Add verification page before unregistering from event from email~~ Add verification page before unregistering from event from email #357 Nov 24, 2019

lioneltrebuchon added 2 commits November 24, 2019 12:38

Reinstore email in notify_signup_accepted

f48523b

Take care of case where event has no date

188548b

lioneltrebuchon force-pushed the master branch from bb7a65b to 188548b Compare November 24, 2019 12:20

NotSpecial suggested changes Nov 24, 2019

View reviewed changes

NotSpecial reviewed Nov 24, 2019

View reviewed changes

Merge branch 'master' into master

1194838

temparus and others added 4 commits December 1, 2019 13:56

Fix newsletter subscription when upgrading users to members via LDAP.

23cbd43

When users are upgraded to members via LDAP, subscribe them to the newsletter. Newsletter settings of already imported members are not touched. Closes amiv-eth#400

Update position field of eventsignups.

1476ecb

- Add documentation for position field. - Add position field to post response. Fixes amiv-eth#405 Fixes amiv-eth#410

Allow admins to overwrite FCFS decision for POST requests.

35d3f16

Even if FCFS in enabled, admins should be allowed to force a signup to be accepted. This worked as intended for PATCH requests already, and now also works for POST. Fixes amiv-eth#381

Merge branch 'master' into master

6dbf668

lioneltrebuchon force-pushed the master branch 2 times, most recently from ad77da0 to 09ff765 Compare December 1, 2019 16:55

Trial to trigger the registration confirmation per email

f10dc1d

lioneltrebuchon force-pushed the master branch from 09ff765 to f10dc1d Compare December 1, 2019 17:07

Rewrite test_emails

8dbe85f

lioneltrebuchon force-pushed the master branch from d120e26 to 8dbe85f Compare December 1, 2019 17:25

NotSpecial suggested changes Dec 1, 2019

View reviewed changes

	MONGO_HOST = 'mongodb' # or 'localhost' if you run the API from Python
	MONGO_HOST = 'mongodb' # or 'localhost' if you run the database locally

Conversation

lioneltrebuchon commented Nov 23, 2019

Uh oh!

NotSpecial commented Nov 23, 2019

Uh oh!

NotSpecial commented Nov 23, 2019

Uh oh!

NotSpecial left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lioneltrebuchon commented Nov 23, 2019

Uh oh!

NotSpecial left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NotSpecial commented Nov 24, 2019

Uh oh!

lioneltrebuchon commented Dec 1, 2019

Uh oh!

NotSpecial left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

NotSpecial left a comment •

edited

Loading