-
Notifications
You must be signed in to change notification settings - Fork 23
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
Ut uc #285
base: blp_dev
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's also run the full unit test suite on this PR as well to verify the unit tests have reduced on our CI boxes.
@organization = blp_org | ||
else | ||
subdomain = CartoDB.subdomainless_urls? ? request.host.to_s.gsub(".#{CartoDB.session_domain}", '') : CartoDB.subdomain_from_request(request) | ||
@organization = ::Organization.where(name: subdomain).first if subdomain |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What will happen if @organization is null (ie: if subdomain is null?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's an invalid scenario for us, since we always need to have the global scenario, the other flow is for carto. @organization is setup in both flows, bloomberg vs non-bloomberg, so it should always be there.
app/controllers/signup_controller.rb
Outdated
@@ -168,7 +171,14 @@ def load_organization | |||
#subdomain = CartoDB.subdomainless_urls? ? request.host.to_s.gsub(".#{CartoDB.session_domain}", '') : CartoDB.subdomain_from_request(request) | |||
#@organization = ::Organization.where(name: subdomain).first if subdomain |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we remove these comments since they are active below now?
@@ -197,7 +197,7 @@ def enqueue_creation(current_controller) | |||
puts "user-auto-creation : save the user_creation info" | |||
|
|||
blp_user_info = build_blp_user_info | |||
blp_user_info.save | |||
blp_user_info.save if blp_user_info.uuid |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the impact if blp_user_info is not saved to the db?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the user creation doesn't have the record during the creation, the user creation fails for bloomberg flow. From the tests, the carto flow should be good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add additional unit tests for the Bloomberg flow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
short answer is 'yes'. I will take it up as a next filler item.
We use the username via http headers