Update account settings page for enterprise learners by zubair-arbi · Pull Request #16703 · openedx/openedx-platform

zubair-arbi · 2017-11-29T11:37:25Z

ENT-762
@asadiqbal08 @saleem-latif @douglashall

Update account settings page:

new help text for fields Username, Full Name, Email Address and Password
display info message Your profile settings are managed by {ENTERPRISE}. Contact your administrator or {edX Support} for help to enterprise learners instead of subtitle for Tab Basic Account Information.
make fields Username, Full Name, Email Address (Sign In) and Country readonly for the enterprise learners
Update styling for the new readonly fields

Remaining tasks:

Update tests [bokchoy, unittests, frontend]

Sandbox:
https://business.sandbox.edx.org/account/settings
credentials: sappluto@yopmail.com / edx

UmanShahzad · 2017-11-29T12:12:00Z

@zubair-arbi FYI, https://github.com/edx/edx-platform/pull/16624 is merged 👍

asadiqbal08 · 2017-12-05T08:23:21Z

            userPreferencesModel.url = userPreferencesApiUrl;

-            if (allowEmailChange) {
+            if (syncLearnerProfileData && enterpriseName) {


Have you written/updated the Jasmine tests for these changes ?

I am updating the jaamine tests.

asadiqbal08 · 2017-12-05T08:23:25Z

+        sync_learner_profile_data = identity_provider.sync_learner_profile_data if identity_provider else False
+
+    context['sync_learner_profile_data'] = sync_learner_profile_data
+    context['edx_support_url'] = settings.SUPPORT_SITE_LINK


should we get the value first from site configuration here ?
configuration_helpers.get_value('SUPPORT_SITE_LINK', settings.SUPPORT_SITE_LINK)

asadiqbal08 · 2017-12-05T08:23:28Z

+    enterprise_customer_name = None
+    sync_learner_profile_data = False
+    enterprise_learner_data = get_enterprise_learner_data(site=request.site, user=request.user)
+    if enterprise_learner_data and 'enterprise_customer' in enterprise_learner_data[0]:


Is that possible a Learner is not associated with enterprise customer in fetch results?
why is it necessary to check ---- 'enterprise_customer' in enterprise_learner_data[0] ?

I am not sure if it is possible for a Learner which is present in enterprise API, to not link with enterprise customer. It seems we are not checking this condition on other places so I am going to remove it.

asadiqbal08 · 2017-12-05T08:23:40Z

                        valueAttribute: 'email',
                        helpMessage: StringUtils.interpolate(
-                            gettext('The email address you use to sign in. Communications from {platform_name} and your courses are sent to this address.'),  // eslint-disable-line max-len
+                            gettext('You receive messages from {platform_name} and course teams at this address.'),  // eslint-disable-line max-len


Is that message pointing to current or future state ?

This text is provided by UX team.

asadiqbal08 · 2017-12-05T08:57:25Z

-                                    gettext('When you select "Reset Your Password", a message will be sent to the email address for your {platform_name} account. Click the link in the message to reset your password.'),  // eslint-disable-line max-len
-                                    {platform_name: platformName}
-                                )
+                                helpMessage: gettext('Check your email account for instructions to reset your password.')  // eslint-disable-line max-len


just a suggestion, should we point out the user current email id in that message too for better readability?
as an example, in most websites I saw the user email address too e.g. password/username reset etc etc.

the email address will be visible in its related email field.

may be we can take suggestion from product owner here? It is just a suggestion.

This help text will be displayed on account settings pages where a learner can view all field including the email address. Please check sandbox https://business.sandbox.edx.org/account/settings.

ok got it. Thanks

asadiqbal08 · 2017-12-05T08:59:17Z

+        syncLearnerProfileData,
+        enterpriseName,
+        enterpriseReadonlyAccountFields,
+        edxSupportUrl


question: can we add trailing , here? so next time that line will not be marked dirty.

this is not python code.

zubair-arbi · 2017-12-06T07:31:11Z

@asadiqbal08 I have added jasmine tests for front end, please have another look at the PR.

zubair-arbi · 2017-12-06T11:30:16Z

@georgebabey please have a look on the changes. You can view the changes on business sandbox https://business.sandbox.edx.org/account/settings (credentials: sappluto@yopmail.com / edx).

brittneyexline

I've verified this on the sandbox and it behaves as described. I've read through the code and only have one formatting request, but this LGTM!

brittneyexline · 2017-12-06T21:43:56Z

+
+            if (syncLearnerProfileData && enterpriseReadonlyAccountFields.fields.indexOf('name') !== -1) {
+                fullnameFieldView = {
+                    view: new AccountSettingsFieldViews.ReadonlyFieldView({


seems like there is an opportunity to reduce code duplication here and below by defining the json object outside of the field object initialization, since most of that data is the same.

asadiqbal08 · 2017-12-07T07:42:39Z

+                Helpers.expectLoadingErrorIsVisible(accountSettingsView, true);
+            });
+
+


check quality here

asadiqbal08 · 2017-12-07T07:42:42Z

+                AjaxHelpers.respondWithJson(requests, Helpers.createAccountSettingsData());
+                AjaxHelpers.respondWithJson(requests, Helpers.TIME_ZONE_RESPONSE);
+                AjaxHelpers.respondWithJson(requests, Helpers.createUserPreferencesData());
+                AjaxHelpers.respondWithJson(requests, {});  // Page viewed analytics event


Why analytics event required in that test?

These checks were implemented in existing code so I also move them in new tests.

asadiqbal08 · 2017-12-07T07:42:44Z

+                expect(sectionsData[0].fields.length).toBe(7);
+
+                // Verify that username, name and email fields are readonly
+                textFields = [sectionsData[0].fields[0], sectionsData[0].fields[1], sectionsData[0].fields[2]];


Can we map the field indexes to some readable constants names? e.g. Enums

asadiqbal08 · 2017-12-07T07:42:47Z

+                        helpMessage: '',
+                        validValue: Helpers.FIELD_OPTIONS[1][0],
+                        invalidValue1: Helpers.FIELD_OPTIONS[2][0],
+                        invalidValue2: Helpers.FIELD_OPTIONS[3][0],


difficult to understand the values behind [1][0], [2][0] ... etc etc. any other way or comments would be helpful

zubair-arbi · 2017-12-07T13:17:03Z

@asadiqbal08 @brittneyexline I have addressed feedback. Please take another look.

ENT-762

edx-pipeline-bot · 2017-12-08T14:18:14Z

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production on Friday, December 08, 2017.

edx-pipeline-bot · 2017-12-11T15:24:37Z

EdX Release Notice: This PR has been deployed to the production environment.

zubair-arbi force-pushed the zub/ENT-762-update-account-settings-page-for-enterprise-sso branch from 2c40ff7 to 2584e45 Compare November 29, 2017 13:06

zubair-arbi changed the title ~~WIP: Update account settings page for enterprise learners~~ Update account settings page for enterprise learners Nov 30, 2017

zubair-arbi force-pushed the zub/ENT-762-update-account-settings-page-for-enterprise-sso branch 3 times, most recently from 7cb1502 to d835358 Compare December 5, 2017 07:07

asadiqbal08 reviewed Dec 5, 2017

View reviewed changes

zubair-arbi force-pushed the zub/ENT-762-update-account-settings-page-for-enterprise-sso branch from d835358 to 65fc720 Compare December 6, 2017 07:27

georgebabey requested review from douglashall and georgebabey December 6, 2017 14:26

brittneyexline approved these changes Dec 6, 2017

View reviewed changes

asadiqbal08 reviewed Dec 7, 2017

View reviewed changes

asadiqbal08 approved these changes Dec 7, 2017

View reviewed changes

Update account settings page for enterprise learners

2ac544c

ENT-762

zubair-arbi force-pushed the zub/ENT-762-update-account-settings-page-for-enterprise-sso branch from 19dfb98 to 2ac544c Compare December 8, 2017 10:41

zubair-arbi merged commit 979c7cd into master Dec 8, 2017

zubair-arbi deleted the zub/ENT-762-update-account-settings-page-for-enterprise-sso branch December 8, 2017 12:56

		Helpers.expectLoadingErrorIsVisible(accountSettingsView, true);
		});

Uh oh!

Conversation

zubair-arbi commented Nov 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

UmanShahzad commented Nov 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asadiqbal08 Dec 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asadiqbal08 Dec 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asadiqbal08 Dec 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zubair-arbi commented Dec 6, 2017

Uh oh!

zubair-arbi commented Dec 6, 2017

Uh oh!

brittneyexline left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zubair-arbi commented Dec 7, 2017

Uh oh!

edx-pipeline-bot commented Dec 8, 2017

Uh oh!

edx-pipeline-bot commented Dec 11, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

zubair-arbi commented Nov 29, 2017 •

edited

Loading

asadiqbal08 Dec 5, 2017 •

edited

Loading

asadiqbal08 Dec 5, 2017 •

edited

Loading

asadiqbal08 Dec 5, 2017 •

edited

Loading