Conversation
ysiad
commented
Nov 4, 2016
ysiad
commented
- Add feature tests for white list functionality
- Change the privilege of whitelist users that all whitelist users are able to add users and delete users
- Change the whitelist to hold GitHub usernames instead of emails
| @@ -0,0 +1,28 @@ | |||
| class Users::ConfirmationsController < Devise::ConfirmationsController | |||
There was a problem hiding this comment.
this entire file was deleted in develop
| @@ -0,0 +1,32 @@ | |||
| class Users::PasswordsController < Devise::PasswordsController | |||
| # GET /resource/password/new | |||
| # def new | |||
There was a problem hiding this comment.
this file was deleted on develop too
| # before_action :configure_sign_up_params, only: [:create] | ||
| # before_action :configure_account_update_params, only: [:update] | ||
|
|
||
| # GET /resource/sign_up |
| # before_action :configure_sign_in_params, only: [:create] | ||
|
|
||
| # GET /resource/sign_in | ||
| # def new |
| # super | ||
| # end | ||
|
|
||
| # POST /resource/unlock |
There was a problem hiding this comment.
Deleted. Don't know why those files didn't get automatically deleted..
app/models/whitelist.rb
Outdated
| validates_format_of :username,:with => /\A[a-z0-9\-_]+\z/i | ||
|
|
||
| def self.has_username?(username) | ||
| if Whitelist.find_by_username(username).nil? |
There was a problem hiding this comment.
the function body can be rewritten as !Whitelist.find_by_username(username).nil?
| t.timestamps null: false | ||
| end | ||
|
|
||
| <<<<<<< HEAD |
| redirect_to whitelists_path | ||
| end | ||
|
|
||
| def check_if_in_whitelist |
There was a problem hiding this comment.
are we allowing anyone to edit whitelist or only admin?
There was a problem hiding this comment.
By the time our professor said, he meant for everyone in the whitelist. Still it's easy for us to change the privilege settings, however we might need to consider redesign the user model so that the privileges can be easily modified.
features/support/paths.rb
Outdated
| '/users/sign_in' | ||
| when /^the whitelist page/ then | ||
| '/whitelists' | ||
| when /^the add user to whitelist page/ then |
There was a problem hiding this comment.
the whitelist management page ?
features/whitelist.feature
Outdated
| Then I should see "User is deleted successfully." | ||
| And I should not see "test-admin" | ||
|
|
||
| Scenario: Whitelist user delete himself from the whitelist |
There was a problem hiding this comment.
I think the behavior should be if the user deleted himself from whitelist then he should be signed out immediately? or actually we shouldn't allow this self-deleting behavior?
There was a problem hiding this comment.
I tested on my local machine. It get redirected to the projects page, and after you click on log out, you will not be able to login again. My best guess for this is that we might need to add another privilege check on projects page so that the user can get signed out immediately. Before each deletion there will be a pop out alert, so the chance of fat finger is actually lowered.
There was a problem hiding this comment.
instead of adding a check on projects page, we can just delete the user's session right away when he deletes himself?
|
@ysiad merge conflicts needs to be resolved |
|
I'll try to review this tomorrow - apologies for the delay |
| def destroy | ||
| user = Whitelist.find(params[:id]) | ||
| if user.username.eql?(current_user.provider_username) | ||
| flash[:notice] = "We just saved you from being an idiot. " |
There was a problem hiding this comment.
maybe some more explanation about idiocy here?
| @@ -0,0 +1,11 @@ | |||
| class CreateAuthorizedUser < ActiveRecord::Migration | |||
There was a problem hiding this comment.
we generally try and avoid manipulating data in our migrations - maybe put this in seed file?
| @@ -0,0 +1,70 @@ | |||
| @omniauth | |||
There was a problem hiding this comment.
some inconsistent indenting, and the scenarios are rather imperative - any chance we could evolve them towards a more declarative stance?
- http://itsadeliverything.com/declarative-vs-imperative-gherkin-scenarios-for-cucumber
- https://danashby.co.uk/2014/12/19/declarative-or-imperative-how-do-you-write-your-feature-file-scenarios/
also the feature description should tell us why someone wants this feature e.g.
As a projectscope admin
So that I can restrict app access to certain people
I would like to have a whitelist of users who can access the app and restrict everyone else
|
apologies for delay - please see comments embedded |
|
thanks @ysiad - can you address codeclimate issues? |
…ol; 3. delete whitelist check before login;
|
thanks @ysiad - we still seem to have some failing tests and codeclimate issues - do you need help interpreting them? |
4 participants
