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

Request for feedback on invite code feature #194

Merged
merged 23 commits into from Sep 30, 2015

Conversation

Projects
None yet
5 participants
@ayajaff
Contributor

ayajaff commented Sep 8, 2015

Hey @elijh and @azul!

We're Aya and Anke from the Rails Girls Summer of Code and we've been working on the invite code feature for the past few weeks. We wanted to let you know what we've done so far and see if you're happy with the invite code feature as it is. We don't expect you to merge our changes yet because one important feature is still missing (as discussed with Azul).

Here is a list of the changes we made:

  • added invite code field to the sign-up form
  • added validation for invite code
  • added rake tasks so the admins can generate invite codes
  • using the coupon code gem to generate the invite codes

What's missing:

  • allow provider to turn the invite code on and off (this feature is crucial before our code is merged as discussed with Azul)
  • separating srp from our code so we don't have to make changes to srp
  • add some nice-to-have features e.g. use multi-use invite codes, expiration dates for invite code, allowing admins to change code from the web interface

Here's a link to our github issues where we track what we're working on:
https://github.com/Alster-Hamburgers/leap_web/issues

Please let us know how you like it and if you have any feedback. We still have a few more weeks left to work on the missing features and implement changes.

Best,
Anke and Aya

@azul

This comment has been minimized.

Member

azul commented Sep 8, 2015

Yay!

I'm really busy right now ⌚️ ... but i'll make sure to go through it in the coming days. Happy to see the build is green 💚.

@ayajaff

This comment has been minimized.

Contributor

ayajaff commented Sep 8, 2015

Okay! If you like we can split it up into smaller chunks? Maybe that's better? :)

@azul

This comment has been minimized.

Member

azul commented Sep 8, 2015

Hey @ayajaff,
Usually smaller chunks are better. Yes. If you find things you can easily isolate from the pull request that still make sense it might be a good idea to separate them out and deal with them before hand.

If that's difficult then we can go through the whole pull request at once.
Cheers,
Azul

@azul

View changes

.gitignore Outdated
@@ -40,3 +40,6 @@ config/customization/*
config/transifex.netrc
!config/provider/.gitkeep
!config/customization/.gitkeep
# ignore IDE configuration
.idea/

This comment has been minimized.

@azul

azul Sep 8, 2015

Member

What IDE is this for?

Maybe this would better fit into a global gitignore on your machine?
If not it might be a minimal pull request on it's own as it's independent of the rest.

This comment has been minimized.

@ankonym

ankonym Sep 8, 2015

Contributor

It's for RubyMine.
True, we actual did add that to our global gitignore, so it doesn't really need to be here.
(Now we just have to find out how to remove it. :) )

This comment has been minimized.

@azul

azul Sep 8, 2015

Member

I bet there are a lot of different ways.
First you might need to know which commit changed it...
Then you can use an interactive rebase and edit the commit in question. Or you could add a new commit and then squash it with the old commit in an interactive rebase.

git log  -1 .gitignore
git rebase -i
@kaeff

This comment has been minimized.

Contributor

kaeff commented Sep 9, 2015

We updated the pull request:

  • Removed the RubyMine-specific ignores as @azul pointed out
  • made the history a little easier to process by squashing related commits

Sorry for the slight confusion of the broken build, I accidentally missed a commit.

class InviteCodeValidator < ActiveModel::Validator
def validate(user)
user_invite_code = InviteCode.find_by_invite_code user.invite_code

This comment has been minimized.

@azul

azul Sep 10, 2015

Member

I had another look at the issue where find_by_invite_code(nil) returning the first record in the table. Turns out this was a couchrest model bug. Luckily it's fixed in the new version. #195 will fix this for leap_web.
So we can safely rely on this to not return an invite code even if the user somehow manages to make user.invite_code return nil. 😅

user_params(login: login, password: password)
assert(@user = User.find_by_login(login), 'user should have been created: %s' % last_response_errors)
user_params(login: login, password: password, invite_code: invite_code)
assert(@user = User.find_by_login(login), 'user should have been encode_credentialseated: %s' % last_response_errors)

This comment has been minimized.

@azul

azul Sep 10, 2015

Member

line 44 looks like an accidental change to me ♻️

This comment has been minimized.

@ayajaff

ayajaff Sep 10, 2015

Contributor

oh oops, you're right. it should've been "user should have been created" :)

@azul

This comment has been minimized.

Member

azul commented Sep 10, 2015

As I already said in Hamburg, I am very happy with the approach. You are aware of the missing parts yourself. So I have nothing to add in terms of the big picture. This leaves different possibility for using the counter. We can decide upon that later. 🍹

Code also looks good to me. I will add a bunch of comments and questions about small things I noticed. They are meant as style suggestions. I'm not sure i follow them myself all the time 😉

timestamps!
design do
view :by__id

This comment has been minimized.

@azul

azul Sep 10, 2015

Member

interesting. are we using this anywhere?

This comment has been minimized.

@ankonym

ankonym Sep 10, 2015

Contributor

Not anymore - we briefly played with the idea to use the id as the invite code but it didn't seem practical (for starters, printed out and then copied down, it wasn't as user-friendly as the coupon code we generate now).
Should we remove it?

This comment has been minimized.

@azul

azul Sep 10, 2015

Member

Yes. Please remove it. It's a quick change and it takes much longer to figure out why this is there later on.

def initialize(attributes = {}, options = {})
super(attributes, options)
write_attribute('invite_code', CouponCode.generate) if new?

This comment has been minimized.

@azul

azul Sep 10, 2015

Member

Cool. read_only and then use write_attribute. Took me a second to figure out why not self.invite_code =
Seems like a good idea.

@@ -0,0 +1,29 @@
class InviteCodeValidator < ActiveModel::Validator
def validate(user)

This comment has been minimized.

@azul

azul Sep 10, 2015

Member

This function could be tweaked a bit for clarity i think.
You fetch the record first. Then hand the code (not the record) to not_existent? and then hand an attribute of the record to count_greater_than_zero?

I think it would be more consistent to only use the code OR the record in this function so the private functions have a similar interface:

if not_existent?(invite_code)
  add_error_to_user("This is not a valid code", user)
elsif used_already?(invite_code)
  add_error_to_user("This code has already been used", user)
end

(I put invite_code as a placeholder for either user.invite_code or user_invite_code. The point is using the same. You would have to adjust the private methods of course.)

This comment has been minimized.

@ankonym

ankonym Sep 10, 2015

Contributor

Hmmm, good point, but I'm still trying to understand why the current version works but using user_invite_code for both returns a nil:NilClass error when I change the code to

if not_existent?(user_invite_code.invite_code) 

rather than

if not_existent?(user.invite_code)

Shouldn't they be the same?

(I don't think we can use user.invite_code for both because used_already? needs the invite_count, and user doesn't have one.)

This comment has been minimized.

@azul

azul Sep 10, 2015

Member

Shouldn't they be the same?

Only if the invite code actually exists. If it does not exist user.invite_code can be anything (it's what the user typed in a form) - and user_invite_code will be nil.
So if you fetch the invite_code and then hand the record to not_existent? you will only need to check for .nil? - no need to try and load the record another time.

(I don't think we can use user.invite_code for both because used_already? needs the invite_count, and user doesn't have one.)

Right. So the fetching of user_invite_code from line 4 would have to move into the used_already? function. We're only using it in that function anyway.

@@ -8,7 +8,7 @@ class User < CouchRest::Model::Base
property :password_salt, String, :accessible => true
property :contact_email, String, :accessible => true
property :contact_email_key, String, :accessible => true
property :invite_code, String, :accessible => true

This comment has been minimized.

@azul

azul Sep 10, 2015

Member

This probably means we store the invite code with the user, right?

I think we should only keep it during registration and then forget about it. But I am not sure. @elijh, what do you think? Do we want to know which user used which invite code?

This comment has been minimized.

@azul

azul Sep 30, 2015

Member

@elijh this is the only open question for me right now. Do we need to store the invite code used?

desc "Generate a batch of invite codes"
task :generate_invites, [:n] => :environment do |task, args|

This comment has been minimized.

@azul

azul Sep 10, 2015

Member

I did not know one can hand args to rake tasks. Nice one!

@kaeff

This comment has been minimized.

Contributor

kaeff commented Sep 21, 2015

As of 3c13802, we saw sign up working with invite codes as well as without. We will apply more testing locally as well as running within the platform tomorrow. Since this is the last week of RGSoC for @ayajaff, we would like to get towards a merge soon given there are no concerns..

@azul Please let us if anything's missing functionally in order to integrate
@varac Let us know when you feel ready for the platform session :)

ankonym and others added some commits Aug 5, 2015

Remove change password browser test
Remove the change password test because the change password functionality is currently unused - however, it breaks with the new invite code field in the signup form.
Make sure codes can only be used once, fix validations
We introduced a count on invite codes to make sure that (at the moment) codes can only be used once. (The code will also allow multi-use codes in the future.)

Also, some of our validations weren't validating against the correct data, which is now fixed.
Fixes for the invite code validator
Validation should only happen for new records
User invite code was nil for invalid invite codes
Adding missing tests
Fix three unit tests by passing Factory Girl a valid invite code
The tests were failing because of a hardcoded "testcode" string so during test setup we generate a valid code and pass it to Factory Girl

ayajaff and others added some commits Sep 4, 2015

Make invite code configurable
Through the config param 'invite_required', providers can decide whether users need to provide an invite code upon signup.
The default setting is false.
Add localization labels to signup form and user.en.yml
Added the necessary labels to allow the localization of the signup form
and the labels to users.en.yml for localization
@azul

This comment has been minimized.

Member

azul commented Sep 30, 2015

@ayajaff @ankonym @kaeff @varac, this looks good to me. 👍
The only issue we still need to clearify is if we want to store the invite code used or not. But we can do that post merge.
Is this ready to be merged from your point of view?

azul added a commit that referenced this pull request Sep 30, 2015

Merge pull request #194 from Alster-Hamburgers/feature/invite_code
Request for feedback on invite code feature

@azul azul merged commit d45f6c6 into leapcode:develop Sep 30, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ankonym ankonym deleted the Alster-Hamburgers:feature/invite_code branch Sep 30, 2015

@elijh

This comment has been minimized.

Member

elijh commented Sep 30, 2015

I also have been sick lately, so I never got around to reading this PR. I am happy it is merged, although I would like to suggest some changes for the future:

(1) Remove the coupon gem and instead generate codes with something like this:

Base32.encode(SecureRandom.random_bytes).downcase[0..7]

or

Base64.encode64(SecureRandom.random_bytes).downcase.gsub(/[0oi1\+_\/]/,'')[0..7]

The first requires the gem base32, but the latter uses only built-in libraries (require 'base64' and require 'securerandom'). This seems better than depending on the coupon gem, especially since we don't use any of the "features" of the coupon gem.

(2) instead of making InviteCode have a "count" property, how about a "uses_remaining" property? This way, in the future, to support invite codes with multiple uses we just change the initial value to be something greater than 1.

(3) It has been useful for riseup.net to store the invite code that is used by a user, in order to shut down spammers. For privacy reasons, however, LEAP probably doesn't want to force providers to keep this information around permanently. How about a rake task that removes the invite_code from all users that were created longer than 3 months ago?

@ankonym

This comment has been minimized.

Contributor

ankonym commented Oct 1, 2015

@elijh Thank you for your comments!

Re: 1 - Understood about not depending on another gem. One thing I liked about the coupon code gem was the code format, which is easier to read and copy off paper for a human than a non-formatted string. This may be helpful should any providers print out codes and hand them off to new users?

Re: 2 - Yes, we were wondering about the best way to implement this.
The possible options we came up with were to either introduce an additional "maximum_uses" property and then count up until count equals maximum uses (this way admins could keep track of how often a certain code has already been used - not sure that use case is something that would come up), or to have a "uses left" property and count down as you described.
Also, the question came up if this could create issues for providers who already generated invite codes with the current system because all those codes would then become invalid - what would be the best way of avoiding problems there?

@kaeff

This comment has been minimized.

Contributor

kaeff commented Oct 1, 2015

On 1 Oct 2015, at 16:05, ankonym notifications@github.com wrote:

Also, the question came up if this could create issues for providers who already generated invite codes with the current system because all those codes would then become invalid - what would be the best way of avoiding problems there?

I’d say that if we implement this in the near future, is safe enough to assume that only pixelated’s wazokazi provider will have invite codes stored that use the old way

@elijh

This comment has been minimized.

Member

elijh commented Oct 1, 2015

One thing I liked about the coupon code gem was the code format, which is easier to read and copy off paper for a human than a non-formatted string. This may be helpful should any providers print out codes and hand them off to new users?

Base64.encode64(SecureRandom.random_bytes).downcase.gsub(/[0oil1+_/]/,'')[0..7].scan(/..../).join('-')

=> "myk9-qjdb"

We may want the length configurable in the future, but I think anything longer than 8 characters is unnecessary.

@elijh

This comment has been minimized.

Member

elijh commented Oct 1, 2015

The possible options we came up with were to either introduce an additional "maximum_uses" property and then count up until count equals maximum uses (this way admins could keep track of how often a certain code has already been used - not sure that use case is something that would come up)

Yes, I think that is better than uses_remaining. I retract what I said.

@ankonym

This comment has been minimized.

Contributor

ankonym commented Oct 2, 2015

Great, I implemented the maximum uses today (here). I'll send a pull request on Monday and then look at the other tasks.

azul added a commit to azul/leap_web that referenced this pull request May 1, 2016

Version 0.8.0
This version ships with improvements implemented during
rails girls summer of code 2015 (in no particular order):
* Providers now can require invite codes
* Admins can disable and enable users
* Payments and subscriptions are possible

Thanks heaps to @ankonym, Aya, @claucece and @EvyW.
Also thanks a lot to rails girls summer of code and thoughtworks for
the organization and coaching.

We also include a bunch of smaller bugfixes. For details see the list
below:

Bugfixes to prepare for 0.8.0 release:
 * upgrade: couchrest_session_store to 0.3.1
 * remove outdated couchrest hack
 * allow monitor auth to create users even if invites are normally required.
 * disable per-user message tests (since this feature currently disabled)
 * api: added super simple motd, closes #7866

Add api support for admin authentication tokens:
 * api: return proper 404 for GET /1/identities/:id.json
 * api: added json error pages, allow "." in the :id param of all api routes
 * api: added get(:show) to identities and users, allow monitors to create/delete test & tmp users.
 * api: added allow ability to limit what IPs can access api using a static configured auth token.
 * api tokens - clarify terms: "monitors" are admins that authenticated via api token, "tmp" users are users that exist only in tmp db, "test" users are either tmp users or users named "test_user_x"
 * api tokens: allow for special api tokens that work like session tokens but are configured in the static config, to be used for infrastructure monitoring.

Upgrade to latest rails 3.2:
 * upgrade: downgrade rake to 10.x
 * upgrade: use latest rails 3.2 version

Smaller fixes:
 * Handle conflict on token cleanup - fixes #7670
 * updated changes file
 * added travis build status to readme
 * allow user accounts to be re-enabled, and for associated identities to also get re-enabled.
 * use RUBY_VERSION instead of :platform for Gemfile (since jessie has a really old bundler)
 * disable failing cucumber test (leap_web is doing the right thing, the test is just weird).
 * internet says that bundler on travis might be what is causing test fail, so force install the lastest one.
 * remove cert fingerprints for disabled users, so that they cannot send email anymore. closes #7690
 * vendor certificate_authority, because travis does not like pulling it from github.
 * travis ci does not support :platform => :ruby_22, so remove for now.
 * enable byebug for tests
 * change the default of config.assets.debug for development env.
 * fix ticket display bug
 * retain locale in URL when logging in and signing up, and ajax actions in general.

Admin UI overhaul:
 * added UI for invite codes
 * added caution tape img.
 * highlight admin areas with caution tape (wip)
 * fix user list

 Generate Invite Codes without code_coupon gem:
 * Cleaned up last traces of the Great Git Mess
 * Remove Coupon Code gem and make invite code = id
 * Replace Coupon Code gem for invite code creation
 * Remove Coupon Code gem and make invite code = id
 * Remove Coupon Code gem from Gemfile
 * Replace Coupon Code gem for invite code creation
 * Fix the InviteCode initialize method so leap_web tests stay green
 * Adjust the rake task to make id = invite code
 * set rbenv pin to 2.1.5

Pull request leapcode#204 from pixelated/fix_payment_check
 * [bug] Only show donation if payment present

Payment and Subscriptions (Pull request leapcode#198 from claucece/develop):
 * Reverting submodule update
 * updated version of fakebraintree
 * changed capybara time
 * fixed gem file
 * add test to payments and subscriptions
 * deleted comment
 * questions added
 * readme
 * updated readme
 * add a comment regarding home
 * update to haml, created translations, deleted files
 * add subscriptions
 * add subs_index and start show
 * changed routes and links
 * subscriptions, translation
 * subscriptions, haml and translations
 * added customers, recurring payment and payment_info
 * just played a little
 * correctly set up comments
 * added payment_info, _customer_form, sucess instances
 * Donation button
 * add donate button, bitcoin, payment_method
 * implemented the form and the generate

Enable/Disable users as admin (Pull request leapcode#196 from EvyW/develop):
 * identing 2
 * Identing first line
 * with out identing
 * Translation changes
 * changes style sheets
 * index changes
 * commit user haml
 * Update leap.scss
 * test users_controller_test
 * fixing translations
 * spanish translations for user actions
 * adding ability to disable/enable users by admin

Allow invites for multiple people:
(Pull request leapcode#201 from Alster-Hamburgers/multi-invite)
 * Adjusted the rake task with comments by @azul
 * Small code cleanup in the rake task
 * Cleaned up invite code output for platform tests
 * Adjust rake task with renamed max_uses
 * Integrated feedback on multi-invite codes
 * Update rake task to allow generation of multi-use invites
 * Allow multi-use invite codes

Pull request leapcode#200 from Alster-Hamburgers/localization
 * Add the localization keys for invite_code and password confirmation

Require invite codes for signup based on config setting:
(Pull request leapcode#194 from Alster-Hamburgers/feature/invite_code)
 * Add localization labels to signup form and user.en.yml
 * Make invite code configurable
 * Cleaned up code in invite_code_validator.rb
 * Removed the view_by__id from invite code test
 * Fixed the signup bug that wrongly consumes the invite code.
 * Fix cucumber tests by passing valid invite code
 * Fix the remaining failures/errors in our tests
 * Fix three unit tests by passing Factory Girl a valid invite code
 * Fix several test failures by stubbing invite code validation
 * Separate user and invite code validator tests
 * Fixes for the invite code validator
 * Make sure codes can only be used once, fix validations
 * Add rake task for invite code batch generation
 * assign random invite code when creating new invite codes
 * Remove change password browser test
 * Fix test based on actual invite code validation
 * Changed invite code query to look for invite_code string instead of id
 * Add validation of invite code in user object based on codes in couch db
 * Add invite code model
 * Added an 'invite code' to all the tests for the sign-up form so we have a valid user for the tests again
 * Update submodule srp to 9e1a41733
 * Move account form info from srp_js into leap_web
 * Adding invite code field to signup with validation for hardcoded invite code
 * Disable CSRF token verification on ticket creation.

Fix issues found during start of rails girls summer of code:
 * couchrest_model 2.0.1 fixes find_by_sth(nil)
 * Update README with docs from website & instructions on local DB
 * Bump therubyracer to 0.12.2
 * improved README.md
 * do not include random cruft in the common name of smtp client certificates

@azul azul referenced this pull request May 1, 2016

Merged

Version 0.8.0 #216

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment