Beta test registration

[omarkhan]

This pull request adds a registration form for the OpenCraft beta. The form is available at /beta/register/.

Testing

• Checkout this branch
• Run the migrations
• In a separate terminal window inside the development VM, start a dummy smtp server with sudo python -m smtpd -n -c DebuggingServer localhost:25
• Start the development server
• Navigate to http://localhost:5000/beta/register/
• Play around with the input fields. Error messages should appear for invalid input
• Submit the form. You should see 2 verification emails in the smtp server output
• Follow the links in the verification emails
• In the django admin, check that a registration, user, and profile have been created, and that the email addresses have been verified

Note that I have not done any styling. This will be done by a web design agency next month.

[omarkhan]

@smarnach this is ready for a look now.

[omarkhan]

Oops, @itsjeyd this one is for you

[smarnach]

Why is this necessary? I think fields are rendered in the order in which you define them in the form class, so you could just define them in the correct order. (I vaguely remember they keep a global counter when a Field class is instantiated, and use that global counter for ordering.)
In any case, you don't need to copy self.fields, since you don't even modify it, and even if you did that would be OK, since the fields shared between all instances of the class are in self.base_fields.

[smarnach]

I see, some of the fields come from the model, and some of the fields are defined here, that makes sense.

(I just had a quick look at the code because I'm curious, by the way. I'm not actually doing a review.)

[omarkhan]

I'm having some trouble writing js tests. The way they are currently set up, they are run completely in isolation, without any of the html from the django templates. This doesn't really make sense here, as most of the functionality I want to test relates to the form, and what django-angular does with it. @itsjeyd @smarnach @antoviaque what should I do here? Just skip these and focus on selenium tests? Extend the js tests to include a fake DOM with something like jsdom?

[itsjeyd]

@omarkhan This should probably be updated to say 2015-2016 (also for the other files you added).

[smarnach]

Do we really need copyright notes in every file? Google stopped doing this since their lawyers said that they don't have any legal relevance whatsoever, and that having a single license file in the project achieves exactly the same. (I can't judge whether that's true, but I find having these redundant comments everywhere rather annoying.)

[bradenmacdonald]

I know Xavier likes these but I feel the same way :)

[antoviaque]

I don't know either, but in doubt on licensing issues, I would pick the opinion of the FSF over the one from Google. Here in any case it seem minor, and it doesn't have much inconvenience to it. I don't personally see that section negatively - it's a place we talk to other devs or companies looking to work on it, and can even be part of the visual esthetic : )

However we should change the contact email - to contact@opencraft.com : )

[smarnach]

All the section on how to apply the licence says is

It is safest to attach them to the start of each source file to most effectively state the exclusion of warranty.

It doesn't say this is necessary for the licence to be valid or anything.

(And I wouldn't trust legal advice from the FSF too much. It seems to be generally agreed that substantial parts of the AGPL are unenforcable under US law; at least that's what I heard from an FSF member.)

[omarkhan]

Whatever we decide here, can we do it in a separate PR?

[itsjeyd]

@omarkhan I'd be fine with that, since we'll also have to update the notices in existing files. Let's see what @antoviaque and @smarnach think.

[smarnach]

My suggestion would be to change the email address and remove the year in a separate PR. The year is bound to get outdated anyway.

[antoviaque]

Yup, updating the email (to contact@opencraft.com) in a separate PR works for me.

[antoviaque]

Done in cb0cf5d . Updating the year only takes running a sed command, so I've left it, and changed it to 2015-2016 in all files.

[smarnach]

@omarkhan Sorry, I can't help regarding the question how to implement JS tests for this, that's a topic I should learn more about.

[antoviaque]

@omarkhan

I'm having some trouble writing js tests. The way they are currently set up, they are run completely in isolation, without any of the html from the django templates. This doesn't really make sense here, as most of the functionality I want to test relates to the form, and what django-angular does with it. @itsjeyd @smarnach @antoviaque what should I do here? Just skip these and focus on selenium tests? Extend the js tests to include a fake DOM with something like jsdom?

I'm not completely sure I understand what is blocking you - you have the dom of the browser? But yes you need to test the result of the operations based on changes on a DOM fixture: https://github.com/open-craft/opencraft/tree/master/instance/tests/fixtures . Here at first sight you would probably want to mock the responses the Django server sends to the client.

In any case please don't add any node/npm dependency, I beg you : )

[omarkhan]

@itsjeyd this is ready for another look now. I have addressed your comments, as well as @smarnach's and @antoviaque's. @antoviaque wants me to contribute the email verification functionality to an existing lib and use that instead, so that's next on my list.

I have added js tests and selenium tests. The js tests will run when you run make test_js. For the selenium tests, run make test_browser.

[omarkhan]

I can't reproduce a bug @itsjeyd reported. Can someone else see if they can reproduce it?

• Type some text into an input box
• Select the text and cut it. 'This field is required' should appear
• Paste the text back into the input box using the keyboard

For @itsjeyd the 'This field is required' message is still displayed. For me it disappears, as expected:

[itsjeyd]

@omarkhan Nit: Missing space between () and {

[antoviaque]

@omarkhan congratz on getting the Js tests done :) Now you still need to add an integration test, you know that, right? :)

@open-craft/core I meant to mention this in the meeting yesterday, but here is just as well - when you send a PR sharing open craft im, please consider adding an integration test. Sometimes it might not make sense to do one, but then please explain the rationale for it in the PR description. The coverage of integration tests is quite low at the moment, we should be careful to add them, especially for user facing features. Otherwise soon we'll be needing QA testers :)

[antoviaque]

When is this run?

It would be good to test the expected results more - currently I only see testing the activation status of a button, and that the next page loads. What page should load, What messages should be displayed in each step?

[antoviaque]

@omarkhan My bad, I had missed them - that will teach me to reply on my phone : ) Cool, thanks. I was looking for an integration folder, but it makes sense to handle the browser tests separately from the server integration tests. I had a few questions/comments, which I posted inline in that code.

[omarkhan]

@itsjeyd this is ready for a final review now.

[itsjeyd]

@omarkhan Can you update this to match the new wording? I.e., "... and occasionally send me an email about it." (I think the help texts for the other fields should still match but I didn't double-check.)

[omarkhan]

Good catch @itsjeyd, I had forgotten to update the migration. Fixed

[smarnach]

@omarkhan I cannot reproduce the issue with "This field is required" continuing to be displayed even when there is data in the input field again. I tried on Firefox 46 and Chrome 49, and tried pasting by Keyboard short cut and context menu as well as Linux middle-click pasting. All combinations worked fine, and I couldn't reproduce any issue.

[omarkhan]

Thanks @smarnach. @itsjeyd OK to merge once I get a green build and squash the commits?

[itsjeyd]

@omarkhan

OK to merge once I get a green build and squash the commits?

Yup, this looks great 👍

[antoviaque]

@omarkhan I still need to do a product review before the merge given that there is user-facing functionality - I've planned it for today though.

[omarkhan]

@antoviaque OK. I have squashed the commits.

[antoviaque]

@omarkhan Looks good! Kudos, this is nice both from the technical and user perspective : )

The comments I had while testing were:

• "Please use a stronger password." => do we state anywhere what the definition of a strong password is? It should be added both to the description on the right-hand column, and to the error message.
• With the default dev environment configuration, submitting the form crashes with https://i.imgur.com/c1F4XcD.png - and the only way to go around it is to setup proper SMTP credentials, and changing the from email to match what is accepted by this mailer. Could you add EMAIL_BACKEND to the settings, defaulting to django.core.mail.backends.console.EmailBackend and set it to django.core.mail.backends.smtp.EmailBackend on console.opencraft.com?
• Currently, the approach is that the submit button is disabled until all the required fields are filled or checked. Which is good, except for the fact that it currently doesn't provide the user with any way to know what is missing, if he doesn't fill all the required fields. For example, only one of the two checkboxes at the end is mandatory - but if I don't check any of them, nothing tells me explicitly which field I am missing, and I can't try to submit it to get an explicit error. I can't think of a good way to provide the user with that feedback at the right time (and only at the right time) if the submit button is disabled - showing it all the time would add a lot of red to the whole form until it is filled; we could also just add a "* Required" label next to all the fields, but the user would still have to look through all of these to search for the one he missed. The only simple and straightforward way that I see is to allow to click on the button when not all the requirements are met, and to show the error message on the missing fields then. That's what was (implicitly) intended in the requirements for "error message at the top of the page, and one next to the issue explaining the issue and how to fix it". But maybe there is a better approach?

[omarkhan]

@antoviaque I have addressed your feedback, can you take another look?

[antoviaque]

@omarkhan Thank you! I'll try, but I'm not sure I will have time for another review today, especially because that means reprovisioning my vagrant VM. Although you will be updating it to the current version once #6 is merged - ping me when it's done that would help. And if I don't get time to rereview before the meeting, feel free to merge to avoid the spillover - we would then take on any additional changes in a future task.

[antoviaque]

@omarkhan I would have had time now to review, but this still needs a rebase. I'll try to reprovision a VM with your branch in the meantime, but you will need the rebase to merge on time before the meeting - note that it will require rebuilding your VM, so make sure to start it ahead of the meeting, to avoid the spillover.

[antoviaque]

By definition, I meant giving the precise constraints - like the minimum number of characters, if there needs to be a combination of special characters, etc.

[smarnach]

@antoviaque I specifically asked in the task description not to have this kind of restrictions, and instead use a library that estimates password strength based on entropy, since restrictions that require special characters etc. are annoying.

@bradenmacdonald You might want to chime in here. :)

[antoviaque]

@smarnach The problem is that the user needs to know what are the constraints - I hadn't noticed this requirement, or more precisely didn't realize that it meant that we can't have a clear definition for the user of the constraint here. Can we provide an example of what will be considered strong enough, in a reliable manner? Like "For example, a 15 letters word or series of words"? Would that be reliable, or will that depend on the letters?

[omarkhan]

@antoviaque some background if you're interested: https://tech.dropbox.com/2012/04/zxcvbn-realistic-password-strength-estimation/

[smarnach]

Fifteen times the letter a certainly wouldn't be strong enough. I think the nicest feedback we could give would be to include a progress bar that indicates how strong the entered password is, with a mark at the required strength. Otherwise, we can just give the general advice to add more lettters, or a bigger diversity of letters.

[omarkhan]

The js implementation of zxcvbn includes nice user-readable messages explaining how a password can be made stronger. These messages are absent from the python port. We could port them over.

[bradenmacdonald]

Alternately, you could only validate the password strength on the client side for now. If the user deliberately hacks around your JS validation to submit a weak password, it'll be their own fault.

The client form could submit the entropy score along with the password, so that the server can at least verify that the client-side checks seem to have run, and the entropy is reasonable.

That way, you get nice suggestion messages, and don't have to worry about inconsistencies between JS and python implementations.

[antoviaque]

@omarkhan @bradenmacdonald Ah, cool - I've had a look at https://dl.dropboxusercontent.com/u/209/zxcvbn/test/index.html and the messages displayed there would definitely solve the problem on my side. Thanks for this. I let you update OC-1587 once you agree on the exact approach?

[omarkhan]

I agree with Braden. I have updated OC-1587

[antoviaque]

@omarkhan Perfect - thanks for the update!

[antoviaque]

Kudos for the upstream PR to django-angular - quickly merged and released, awesome : )

[antoviaque]

You can use 0.8.2 from jrief/django-angular#256 now, right?

[omarkhan]

@antoviaque rebased. I don't see anything in #6 that would cause any problems with this. Once I get a green build I will merge.

[antoviaque]

@omarkhan Alright - but #59 (comment) still needs to be addressed. Can you have a look at my question there? If there is no time that can be put in a follow-up task, but if it's manageable by a string change it would be simpler to do it now.