-
Notifications
You must be signed in to change notification settings - Fork 1
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] Save contact info after sign up #1130
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.
It looks great! I have a few questions, but I don't want to block you so I will approve the PR.
@@ -1,4 +1,4 @@ | |||
<contact-info submitted="$ctrl.submitted" on-submit="$ctrl.onSubmit(success)"></contact-info> | |||
<contact-info submitted="$ctrl.submitted" on-submit="$ctrl.onSubmit(success)" donor-details="$ctrl.signUpDonorDetails"></contact-info> |
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.
Where is signUpDonorDetails
defined? Were you also meant to commit changes to the contactInfoModal.component.js
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 is set in checkDonorDetails
if there was an error saving the donor details
src/common/components/registerAccountModal/registerAccountModal.tpl.html
Outdated
Show resolved
Hide resolved
e146d50
to
5efb9d2
Compare
5efb9d2
to
1227c65
Compare
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.
Great work with this! I only had comments, nothing bad, just some of my thoughts.
I've approved this to not delay you.
this.getDonorDetailsSubscription = this.orderService.getDonorDetails().flatMap((donorDetails) => { | ||
if (signUpDonorDetails && donorDetails['registration-state'] === 'NEW') { | ||
// Save the contact info from signup | ||
merge(donorDetails, signUpDonorDetails) | ||
|
||
const requests = [this.orderService.updateDonorDetails(donorDetails)] | ||
if (donorDetails.email) { | ||
requests.push(this.orderService.addEmail(donorDetails.email, donorDetails.emailFormUri)) | ||
} | ||
|
||
// Send each of the requests and pass donorDetails to the next step after the requests complete | ||
return Observable.forkJoin(requests).map(() => donorDetails).do({ | ||
error: () => { | ||
// If there was an error, save the donor details from sign up so that they will be added | ||
// to the contact info form. The error handler below will change the step to contact-info. | ||
this.signUpDonorDetails = signUpDonorDetails | ||
} | ||
}) | ||
} | ||
|
||
// Pass donorDetails to the next step | ||
return Observable.of(donorDetails) | ||
}).subscribe({ |
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.
Researching flatMap
I came across switchMap
which could bet better for us to use as it ensures the latest request is processed.
I haven't tried this so it might not work.
this.getDonorDetailsSubscription = this.orderService.getDonorDetails().flatMap((donorDetails) => { | |
if (signUpDonorDetails && donorDetails['registration-state'] === 'NEW') { | |
// Save the contact info from signup | |
merge(donorDetails, signUpDonorDetails) | |
const requests = [this.orderService.updateDonorDetails(donorDetails)] | |
if (donorDetails.email) { | |
requests.push(this.orderService.addEmail(donorDetails.email, donorDetails.emailFormUri)) | |
} | |
// Send each of the requests and pass donorDetails to the next step after the requests complete | |
return Observable.forkJoin(requests).map(() => donorDetails).do({ | |
error: () => { | |
// If there was an error, save the donor details from sign up so that they will be added | |
// to the contact info form. The error handler below will change the step to contact-info. | |
this.signUpDonorDetails = signUpDonorDetails | |
} | |
}) | |
} | |
// Pass donorDetails to the next step | |
return Observable.of(donorDetails) | |
}).subscribe({ | |
this.getDonorDetailsSubscription = this.orderService.getDonorDetails().switchMap((donorDetails) => { | |
if (signUpDonorDetails && donorDetails['registration-state'] === 'NEW') { | |
// Save the contact info from signup | |
merge(donorDetails, signUpDonorDetails) | |
const requests = [this.orderService.updateDonorDetails(donorDetails)] | |
if (donorDetails.email) { | |
requests.push(this.orderService.addEmail(donorDetails.email, donorDetails.emailFormUri)) | |
} | |
// Send each of the requests and pass donorDetails to the next step after the requests complete | |
return Observable.forkJoin(requests).map(() => donorDetails).catch((error) => { | |
// If there was an error, save the donor details from sign up so that they will be added | |
// to the contact info form. The error handler below will change the step to contact-info. | |
this.signUpDonorDetails = signUpDonorDetails | |
return Observable.throw(error); | |
}) | |
} | |
// Pass donorDetails to the next step | |
return Observable.of(donorDetails) | |
}).subscribe({ |
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.
None of the observables emit more than one item, so I don't think it matters, but I can switch it.
Why does your suggestion switch from do
to catch
? I was thinking that do
was nice because you can pass through the same error without having to do return Observable.throw(error)
.
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.
I switched do
to catch
as it was easier to read, but these changes are just suggestions based on preference.
You are correct, you don't need to write Observable.throw(error)
if you write do
src/common/components/registerAccountModal/registerAccountModal.component.spec.js
Show resolved
Hide resolved
e59b733
to
db3f0d3
Compare
db3f0d3
to
2097ab2
Compare
Changes
onStateChange
binding with dedicatedonSignUp
andonSignIn
bindingsI made this a PR instead of committing directly so it's easier for you to see what changed and see if I'm going in the right direction with this.