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

[EP-2362] Deduplicate sign in modals #1132

Merged
merged 6 commits into from
Jan 13, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/app/designationEditor/designationEditor.component.js
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ class DesignationEditorController {
this.updateCarousel()
}, error => {
if (error.status === 422 && !this.retried) {
return this.sessionModalService.open('sign-in', {
return this.sessionModalService.open('register-account', {
backdrop: 'static',
keyboard: false
}).result.then(() => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,13 @@ class RegisterAccountModalController {
}
}

onSignUp () {
this.stateChanged('sign-up')
}

onIdentitySuccess () {
this.sessionService.removeOktaRedirectIndicator()

// Success Sign-In/Up, Proceed to Step 2.
this.getDonorDetails()
}
Expand Down Expand Up @@ -142,10 +148,19 @@ class RegisterAccountModalController {

stateChanged (state) {
this.element.dataset.state = state
if (state === 'sign-up') {
if ((!this.welcomeBack && state === 'sign-in') || state === 'sign-up') {
// Use a small modal for sign in modals without a welcome back message and for sign up modals
// regardless of the screen size because they can't take advantage of the extra width
this.setModalSize('sm')
} else if (this.$window.innerWidth >= 1200) {
// Use a large modal on wide screens for other modals
this.setModalSize('lg')
} else if (state === 'contact-info') {
// Use a medium modal for contact info modals, even on narrow screens, because they need the extra width
this.setModalSize('md')
} else {
this.setModalSize(this.$window.screen.width >= 1200 ? 'lg' : state === 'contact-info' ? undefined : 'sm')
// Use a small modal for all other modals on narrow screens
this.setModalSize('sm')
}

this.state = state
Expand All @@ -159,9 +174,7 @@ class RegisterAccountModalController {
// Modal size is unchangeable after initialization. This fetches the modal and changes the size classes.
const modal = angular.element(document.getElementsByClassName('session-modal'))
modal.removeClass('modal-sm modal-md modal-lg')
if (angular.isDefined(size)) {
modal.addClass(`modal-${size}`)
}
modal.addClass(`modal-${size}`)
}
}

Expand All @@ -183,6 +196,9 @@ export default angular
bindings: {
firstName: '=',
modalTitle: '=',
// If true, show the "Welcome back!" message in the header/sidebar
welcomeBack: '<?',
lastPurchaseId: '<',
onSuccess: '&',
onCancel: '&',
setLoading: '&'
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ describe('registerAccountModal', function () {
$element: [{ dataset: {} }],
orderService: { getDonorDetails: jest.fn() },
verificationService: { postDonorMatches: jest.fn() },
sessionService: {
sessionService: {
getRole: jest.fn(),
isOktaRedirecting: jest.fn(),
removeOktaRedirectIndicator: jest.fn(),
Expand Down Expand Up @@ -250,12 +250,20 @@ describe('registerAccountModal', function () {
})

describe('stateChanged( state )', () => {
let originalWidth

beforeEach(() => {
originalWidth = $ctrl.$window.innerWidth

$ctrl.state = 'unknown'
jest.spyOn($ctrl, 'setModalSize').mockImplementation(() => {})
jest.spyOn($ctrl, 'scrollModalToTop').mockImplementation(() => {})
})

afterEach(() => {
$ctrl.$window.innerWidth = originalWidth
})

it('should scroll to the top of the modal', () => {
$ctrl.stateChanged()

Expand All @@ -273,15 +281,15 @@ describe('registerAccountModal', function () {
it('changes to \'sign-up\' state', () => {
$ctrl.stateChanged('sign-up')

expect($ctrl.setModalSize).toHaveBeenCalledWith('md')
expect($ctrl.setModalSize).toHaveBeenCalledWith('sm')
expect($ctrl.setLoading).toHaveBeenCalledWith({ loading: false })
expect($ctrl.state).toEqual('sign-up')
})

it('changes to \'contact-info\' state', () => {
$ctrl.stateChanged('contact-info')

expect($ctrl.setModalSize).toHaveBeenCalledWith(undefined)
expect($ctrl.setModalSize).toHaveBeenCalledWith('md')
expect($ctrl.setLoading).toHaveBeenCalledWith({ loading: false })
expect($ctrl.state).toEqual('contact-info')
})
Expand All @@ -293,6 +301,61 @@ describe('registerAccountModal', function () {
expect($ctrl.setLoading).toHaveBeenCalledWith({ loading: false })
expect($ctrl.state).toEqual('failed-verification')
})

describe('when welcomeBack is true', () => {
beforeEach(() => {
$ctrl.welcomeBack = true
})

it('sets the sign-in modal size to large on wide screens', () => {
$ctrl.$window.innerWidth = 1200
$ctrl.stateChanged('sign-in')

expect($ctrl.setModalSize).toHaveBeenCalledWith('lg')
})

it('sets the sign-in modal size to small on narrow screens', () => {
$ctrl.stateChanged('sign-in')

expect($ctrl.setModalSize).toHaveBeenCalledWith('sm')
})

it('sets the sign-up modal size to small', () => {
$ctrl.stateChanged('sign-up')

expect($ctrl.setModalSize).toHaveBeenCalledWith('sm')
})

it('sets the contact-info modal size to medium', () => {
$ctrl.stateChanged('contact-info')

expect($ctrl.setModalSize).toHaveBeenCalledWith('md')
})
})

describe('when the screen is wide', () => {
beforeEach(() => {
$ctrl.$window.innerWidth = 1200
})

it('sets the sign-in modal size to small', () => {
$ctrl.stateChanged('sign-in')

expect($ctrl.setModalSize).toHaveBeenCalledWith('sm')
})

it('sets the sign-up modal size to small', () => {
$ctrl.stateChanged('sign-up')

expect($ctrl.setModalSize).toHaveBeenCalledWith('sm')
})

it('sets the contact-info modal size to large', () => {
$ctrl.stateChanged('contact-info')

expect($ctrl.setModalSize).toHaveBeenCalledWith('lg')
})
})
})

describe('setModalSize( size )', () => {
Expand All @@ -309,12 +372,5 @@ describe('registerAccountModal', function () {
expect(modal.removeClass).toHaveBeenCalledWith('modal-sm modal-md modal-lg')
expect(modal.addClass).toHaveBeenCalledWith('modal-sm')
})

it('sets size missing param', () => {
$ctrl.setModalSize()

expect(modal.removeClass).toHaveBeenCalledWith('modal-sm modal-md modal-lg')
expect(modal.addClass).not.toHaveBeenCalled()
})
})
})
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<div class="modal-header {{$ctrl.state}}" >
<div class="modal-header {{$ctrl.state}}" ng-if="$ctrl.state !== 'sign-in' || $ctrl.welcomeBack">
<button type="button" class="close" aria-label="Close" ng-click="$ctrl.onCancel()">
<span aria-hidden="true">×</span>
</button>
Expand All @@ -24,7 +24,8 @@ <h3 translate>Create Your Account</h3>
<sign-in-modal
ng-switch-when="sign-in"
modal-title="$ctrl.modalTitle"
on-state-change="$ctrl.stateChanged(state)"
last-purchase-id="$ctrl.lastPurchaseId"
on-sign-up="$ctrl.onSignUp()"
on-success="$ctrl.onIdentitySuccess()"
on-failure="$ctrl.onIdentityFailure()"
is-inside-another-modal="true"
Expand Down
1 change: 0 additions & 1 deletion src/common/components/signInForm/signInForm.component.js
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,6 @@ export default angular
bindings: {
onSuccess: '&',
onFailure: '&',
onStateChange: '&',
canac marked this conversation as resolved.
Show resolved Hide resolved
lastPurchaseId: '<',
onSignUpWithOkta: '&',
onSignInPage: '<'
Expand Down
7 changes: 2 additions & 5 deletions src/common/components/signInModal/signInModal.component.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,6 @@ class SignInModalController {
getOktaUrl () {
return this.sessionService.getOktaUrl()
}

stateChanged (state) {
this.onStateChange({ state })
}
}

export default angular
Expand All @@ -43,7 +39,8 @@ export default angular
bindings: {
modalTitle: '=',
lastPurchaseId: '<',
onStateChange: '&',
// Called when the user clicks the create account link
onSignUp: '&',
onSuccess: '&',
onFailure: '&',
isInsideAnotherModal: '='
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,11 +58,4 @@ describe('signInModal', function () {
expect($ctrl.sessionService.getOktaUrl).toHaveBeenCalled()
})
})

describe('stateChanged', () => {
it('updates the state value', () => {
$ctrl.stateChanged('newState')
expect($ctrl.onStateChange).toHaveBeenCalledWith({state: 'newState'})
})
})
})
3 changes: 1 addition & 2 deletions src/common/components/signInModal/signInModal.tpl.html
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,7 @@
on-success="$ctrl.onSuccess()"
on-failure="$ctrl.onFailure()"
last-purchase-id="$ctrl.lastPurchaseId"
on-state-change="$ctrl.stateChanged(state)"
on-sign-up-with-okta="$ctrl.stateChanged('sign-up')"
on-sign-up-with-okta="$ctrl.onSignUp()"
/>
</div>
</div>
3 changes: 1 addition & 2 deletions src/common/services/session/sessionEnforcer.service.js
Original file line number Diff line number Diff line change
Expand Up @@ -98,8 +98,7 @@ const SessionEnforcerService = /* @ngInject */ function ($window, orderService,
})

if (angular.isUndefined(modal)) {
const modalType = find(enforced, { mode: EnforcerModes.donor }) ? 'register-account' : 'sign-in'
modal = sessionModalService.open(modalType, {
modal = sessionModalService.open('register-account', {
backdrop: 'static',
keyboard: false
}).result
Comment on lines +101 to 104
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should const modalType = find(enforced, { mode: EnforcerModes.donor }) this define if the register-account modal is minimal

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what you're asking. But now the register-account modal now defaults to minimal mode (welcomeBack is falsy), if that's what you mean.

Expand Down
18 changes: 9 additions & 9 deletions src/common/services/session/sessionEnforcer.service.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -70,9 +70,9 @@ describe('sessionEnforcerService Tests', () => {
}, EnforcerModes.session)
}))

it('opens sign-in modal and calls \'change\' callback', () => {
it('opens register-account modal and calls \'change\' callback', () => {
expect(change).toHaveBeenCalledWith(Roles.public, 'NEW')
expect(sessionModalService.open).toHaveBeenCalledWith('sign-in', { backdrop: 'static', keyboard: false })
expect(sessionModalService.open).toHaveBeenCalledWith('register-account', { backdrop: 'static', keyboard: false })
})

describe('sign-in success', () => {
Expand All @@ -98,13 +98,13 @@ describe('sessionEnforcerService Tests', () => {
sessionService.getRole.mockReturnValue(Roles.registered)
jest.spyOn(sessionModalService, 'currentModal')
})

it('should call \'signIn\' and sessionModalService.currentModal callback', () => {
$rootScope.$digest()
expect(signIn).toHaveBeenCalled()
expect(sessionModalService.currentModal).toHaveBeenCalled()
})

it('should close sessionModalService.currentModal', () => {
let currentModalCloseMock = jest.fn()
jest.spyOn(sessionModalService, 'currentModal').mockImplementation(() => {
Expand All @@ -128,21 +128,21 @@ describe('sessionEnforcerService Tests', () => {
sessionEnforcerService([Roles.registered], {}, EnforcerModes.session)
}))

it('opens sign-in modal and does not call \'change\' callback', () => {
it('opens register-account modal and does not call \'change\' callback', () => {
expect(change).not.toHaveBeenCalled()
expect(sessionModalService.open).toHaveBeenCalledWith('sign-in', { backdrop: 'static', keyboard: false })
expect(sessionModalService.open).toHaveBeenCalledWith('register-account', { backdrop: 'static', keyboard: false })
})

describe('sign-in success', () => {
it('does not call \'sign-in\' callback', () => {
describe('register-account success', () => {
it('does not call \'signIn\' callback', () => {
deferred.resolve()
$rootScope.$digest()

expect(signIn).not.toHaveBeenCalled()
})
})

describe('sign-in canceled', () => {
describe('register-account canceled', () => {
it('does not call \'cancel\' callback', () => {
deferred.reject()
$rootScope.$digest()
Expand Down
14 changes: 1 addition & 13 deletions src/common/services/session/sessionModal.component.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import angular from 'angular'

import signInModal from 'common/components/signInModal/signInModal.component'
import signUpModal from 'common/components/signUpModal/signUpModal.component'
import userMatchModal from 'common/components/userMatchModal/userMatchModal.component'
import contactInfoModal from 'common/components/contactInfoModal/contactInfoModal.component'
Expand Down Expand Up @@ -36,6 +35,7 @@ class SessionModalController {
this.firstName = session.first_name
})
this.stateChanged(this.resolve.state)
this.welcomeBack = this.resolve.welcomeBack
this.lastPurchaseId = this.resolve.lastPurchaseId
}

Expand All @@ -48,17 +48,6 @@ class SessionModalController {
this.scrollModalToTop()
}

onSignInSuccess () {
this.sessionService.removeOktaRedirectIndicator()
const $injector = this.$injector
if (!$injector.has('sessionService')) {
$injector.loadNewModules(['sessionService'])
}
this.$document[0].body.dispatchEvent(
new window.CustomEvent('giveSignInSuccess', { bubbles: true, detail: { $injector } }))
this.close()
}

onSignUpSuccess () {
this.analyticsFactory.track('ga-sign-in-create-login')
this.sessionService.removeOktaRedirectIndicator()
Expand Down Expand Up @@ -87,7 +76,6 @@ class SessionModalController {

export default angular
.module(componentName, [
signInModal.name,
signUpModal.name,
userMatchModal.name,
contactInfoModal.name,
Expand Down
25 changes: 0 additions & 25 deletions src/common/services/session/sessionModal.component.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -80,31 +80,6 @@ describe('sessionModalController', function () {
})
})

describe('$ctrl.onSignInSuccess', () => {
it('should close modal', () => {
jest.spyOn($ctrl.sessionService, 'removeOktaRedirectIndicator').mockImplementation(() => {})
$ctrl.$injector.has.mockImplementation(() => true)
$ctrl.onSignInSuccess()
const $injector = $ctrl.$injector

expect($ctrl.close).toHaveBeenCalled()
expect($ctrl.sessionService.removeOktaRedirectIndicator).toHaveBeenCalled()
expect($ctrl.$document[0].body.dispatchEvent).toHaveBeenCalledWith(
new window.CustomEvent('giveSignInSuccess', { bubbles: true, detail: { $injector } }))
})

it('should add the sessionService module', () => {
$ctrl.$injector.has.mockImplementation(() => false)
$ctrl.$injector.loadNewModules.mockImplementation(() => {})
$ctrl.onSignInSuccess()
const $injector = $ctrl.$injector

expect($injector.loadNewModules).toHaveBeenCalledWith(['sessionService'])
expect($ctrl.$document[0].body.dispatchEvent).toHaveBeenCalledWith(
new window.CustomEvent('giveSignInSuccess', { bubbles: true, detail: { $injector } }))
})
})

describe('$ctrl.onSignUpSuccess', () => {
it('should close modal', () => {
jest.spyOn($ctrl.sessionService, 'removeOktaRedirectIndicator').mockImplementation(() => {})
Expand Down
Loading