Skip to content
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

Set default dashboard for users and fix a bug in preferences for default dashboard. #3781

Merged
merged 9 commits into from Aug 22, 2017

Conversation

Projects
None yet
3 participants
@Arunabh98
Copy link
Contributor

Arunabh98 commented Aug 20, 2017

As decided in our meeting, we no longer need to make a banner as all users have a default dashboard. Users who signup using the create button will have the creator dashboard set as default.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Aug 20, 2017

Codecov Report

Merging #3781 into develop will decrease coverage by 0.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           develop   #3781      +/-   ##
==========================================
- Coverage    45.72%   45.7%   -0.02%     
==========================================
  Files          280     280              
  Lines        21134   21140       +6     
  Branches      3285    3286       +1     
==========================================
  Hits          9663    9663              
- Misses       11471   11477       +6
Impacted Files Coverage Δ
core/templates/dev/head/pages/signup/Signup.js 44.18% <0%> (-3.32%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 384cea3...109392a. Read the comment docs.

@@ -243,6 +245,12 @@ def get(self):
"""Handles GET requests."""
return_url = str(self.request.get('return_url', self.request.uri))

# Check if the return url is the creator dashboard. If it is, we should
# set the default dashboard of the user as the creator dashboard.
if feconf.CREATOR_DASHBOARD_URL in return_url:

This comment has been minimized.

Copy link
@seanlip

seanlip Aug 20, 2017

Member

I think you should do this in a later stage, only after the user has actually signed up. In general, avoid state-changing operations in GET requests.

This comment has been minimized.

Copy link
@seanlip

seanlip Aug 20, 2017

Member

Also ... tests?

@@ -72,13 +72,13 @@
<div class="col-lg-10 col-md-10 col-sm-10">
<div class="checkbox" style="padding-top: 0;">
<label>
<input type="radio" ng-model="defaultDashboard" value="DASHBOARD_TYPE_CREATOR" ng-change="saveDefaultDashboard(defaultDashboard)">
<input type="radio" ng-model="defaultDashboard" value="<[DASHBOARD_TYPE_CREATOR]>" ng-change="saveDefaultDashboard(defaultDashboard)">

This comment has been minimized.

Copy link
@seanlip

seanlip Aug 20, 2017

Member

Optional: would ng-value be better? I'm not sure.

Also, is this actually an error in production -- i.e. is a hotfix needed?

This comment has been minimized.

Copy link
@seanlip

seanlip Aug 20, 2017

Member

Should we also have e2e tests to verify that the default dashboard is changed when the preferences are changed?

urlService.getUrlParams().return_url);


// If the user has the return url as the creator dashboard, it is

This comment has been minimized.

Copy link
@seanlip

seanlip Aug 21, 2017

Member

Why do you not do this in the backend POST request, to avoid a second RPC and a potential race condition?

This comment has been minimized.

Copy link
@seanlip

seanlip Aug 21, 2017

Member

Don't forget the back end tests, too.

This comment has been minimized.

Copy link
@Arunabh98

Arunabh98 Aug 21, 2017

Author Contributor

I am facing with finding the return URL. Unfortunately, in the POST request for the signuphandler I do not have the return url part. Should I pass it as a backend parameter? That would be a lot easier I guess. Right now it's very difficult to what I've implemented. What do you think about this method?

This comment has been minimized.

Copy link
@seanlip

seanlip Aug 21, 2017

Member

I'd say, just pass a variable that's called default_dashboard or similar. The handler doesn't need the full URL.

@@ -124,6 +124,23 @@ oppia.controller('Signup', [
agreed_to_terms: agreedToTerms,
can_receive_email_updates: null
};

var defaultDashboard = '';

This comment has been minimized.

Copy link
@seanlip

seanlip Aug 21, 2017

Member

Suggest defensively programming this so that it defaults to "learner" here. That way the value can never be invalid, even if further logic is added in the future.

This comment has been minimized.

Copy link
@Arunabh98

Arunabh98 Aug 21, 2017

Author Contributor

Done!

can_receive_email_updates = self.payload.get(
'can_receive_email_updates')

has_ever_registered = user_services.has_ever_registered(self.user_id)
has_fully_registered = user_services.has_fully_registered(self.user_id)

# Set the default dashboard for new users.

This comment has been minimized.

Copy link
@seanlip

seanlip Aug 21, 2017

Member

No, this should only happen if the user has never registered in the past, and registration succeeds, right? So basically it should go around or after line 321:

if not has_ever_registered:
    # do the default dashboard stuff

Note that this handler can get called again by existing users if we update the terms and ask them to agree to them. Or if they click the Back button.

This comment has been minimized.

Copy link
@seanlip

seanlip Aug 21, 2017

Member

Also, thinking defensively, could you add a "choices" property to core/storage/user/gae_models.py to restrict the possible values, or some kind of validation in update_user_default_dashboard, or here? Basically, somewhere along the backend pipeline, we should make sure that only allowed values make it through the flow.

This comment has been minimized.

Copy link
@Arunabh98

Arunabh98 Aug 21, 2017

Author Contributor

Done both!

@Arunabh98

This comment has been minimized.

Copy link
Contributor Author

Arunabh98 commented Aug 21, 2017

Also added backend tests.

@seanlip
Copy link
Member

seanlip left a comment

LGTM. Thanks!

@seanlip seanlip added the PR: LGTM label Aug 21, 2017

@seanlip seanlip merged commit dcf6bd0 into oppia:develop Aug 22, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.