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

feat: Add new wizard step - attendee form #2961

Merged
merged 12 commits into from Jun 21, 2019

Conversation

@uds5501
Copy link
Contributor

commented May 19, 2019

Fixes #2963

  • Adds a new step in wizard for ticket-buyer-form

Checklist

  • I have read the Contribution & Best practices Guide.
  • My branch is up-to-date with the Upstream development branch.
  • The acceptance, integration, unit tests and linter pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
@uds5501

This comment has been minimized.

Copy link
Contributor Author

commented May 19, 2019

@fossasia/open-event-frontend-devs, @mariobehling Kindly refrain from updating the branch till the [WIP] tag is intact. Thank you!
I will be posting the screen shot of the first iteration soon.

@fossasia fossasia deleted a comment from codacy-bot May 19, 2019

@fossasia fossasia deleted a comment from codacy-bot May 19, 2019

@uds5501

This comment has been minimized.

Copy link
Contributor Author

commented May 19, 2019

Screenshot from 2019-05-19 23-47-29
@mariobehling Condition of wizard after iteration 1

@uds5501 uds5501 force-pushed the uds5501:enh-2956 branch from fcf4a9b to d55d637 May 19, 2019

@mariobehling

This comment has been minimized.

Copy link
Member

commented May 20, 2019

Please rename "Ticket Buyer Form" to "Attendee Form". Some events don't sell tickets. Also we are collecting the info of attendees not the buyers. (of course they are usually the same, but not necessarily)

@uds5501

This comment has been minimized.

Copy link
Contributor Author

commented May 20, 2019

@mariobehling okay, will be changing the names as required in next commit

@fossasia fossasia deleted a comment from codacy-bot May 20, 2019

@fossasia fossasia deleted a comment from codacy-bot May 20, 2019

@uds5501

This comment has been minimized.

Copy link
Contributor Author

commented May 20, 2019

Corresponding server PR : fossasia/open-event-server#5934

@uds5501 uds5501 changed the title [WIP]feat: Add new wizard step : ticket-buyer-form feat: Add new wizard step - attendee form May 20, 2019

@auto-label auto-label bot added the feature label May 20, 2019

@uds5501

This comment has been minimized.

Copy link
Contributor Author

commented May 20, 2019

@CosmicCoder96
Copy link
Collaborator

left a comment

  • Keep the naming of component similar to the rest of the steps.
app/routes/events/view/edit/ticket-buyer-form.js Outdated Show resolved Hide resolved
@uds5501

This comment has been minimized.

Copy link
Contributor Author

commented May 23, 2019

@CosmicCoder96 Made the required changes.

},
actions: {
save(data) {
this.saveForms(data);

This comment has been minimized.

Copy link
@niranjan94

niranjan94 Jun 18, 2019

Member

Why are you not handling the errors ?

This comment has been minimized.

Copy link
@uds5501

uds5501 Jun 18, 2019

Author Contributor

@niranjan94 The errors are being handled by the functions saveForms and saveEventDataAndRedirectTo themselves.

This comment has been minimized.

Copy link
@niranjan94

niranjan94 Jun 19, 2019

Member

No. They are not. I'm not talking about the form validation errors. Lets say customForm.save fails with an error. How is that being handled ?

This comment has been minimized.

Copy link
@shreyanshdwivedi

shreyanshdwivedi Jun 19, 2019

Member

@uds5501 maybe you can use try-catch to handle such errors

This comment has been minimized.

Copy link
@uds5501

uds5501 Jun 19, 2019

Author Contributor

@niranjan94 @shreyanshdwivedi Alright, will work on it tonight

);
},
move(direction, data) {
this.saveForms(data);

This comment has been minimized.

Copy link
@niranjan94

niranjan94 Jun 18, 2019

Member

Why are you not handling the errors ?

},
actions: {
save(data) {
this.saveForms(data);

This comment has been minimized.

Copy link
@niranjan94

niranjan94 Jun 19, 2019

Member

No. They are not. I'm not talking about the form validation errors. Lets say customForm.save fails with an error. How is that being handled ?

@uds5501

This comment has been minimized.

Copy link
Contributor Author

commented Jun 19, 2019

@mariobehling mariobehling requested a review from mrsaicharan1 Jun 19, 2019

[]
);
},
move(direction, data) {

This comment has been minimized.

Copy link
@mrsaicharan1

mrsaicharan1 Jun 20, 2019

Member

Use try/catch blocks here as well

This comment has been minimized.

Copy link
@uds5501

uds5501 Jun 20, 2019

Author Contributor

@mrsaicharan1 They already have error handling implemented. (see EventMixin)

data.customForms = await data.event.query('customForms', {
filter : filterOptions,
sort : 'id',
'page[size]' : 50

This comment has been minimized.

Copy link
@mrsaicharan1

mrsaicharan1 Jun 20, 2019

Member

Is the page[size] same for other forms as well?

This comment has been minimized.

Copy link
@uds5501

uds5501 Jun 20, 2019

Author Contributor
export default Controller.extend(EventWizardMixin, {
async saveForms(data) {
for (const customForm of data.customForms ? data.customForms.toArray() : []) {
try {

This comment has been minimized.

Copy link
@niranjan94

niranjan94 Jun 20, 2019

Member

@uds5501 instead of doing it here for each loop, do it at the point where you call saveForms. That way even if one fails, others after stop.

This comment has been minimized.

Copy link
@uds5501

uds5501 Jun 20, 2019

Author Contributor

@niranjan94 Okay!

This comment has been minimized.

Copy link
@uds5501

uds5501 Jun 20, 2019

Author Contributor

@niranjan94 fixed the implementation, please review

} catch (error) {
this.notify.error(this.ln10.t(error.message));
}
this.saveEventDataAndRedirectTo(

This comment has been minimized.

Copy link
@niranjan94

niranjan94 Jun 20, 2019

Member

Do not do this if there was an error.

This comment has been minimized.

Copy link
@uds5501

uds5501 Jun 20, 2019

Author Contributor

@niranjan94 What should be done in case of error? 😅

This comment has been minimized.

Copy link
@shreyanshdwivedi

shreyanshdwivedi Jun 20, 2019

Member

@uds5501 you can shift function on line 29 to try block.

This comment has been minimized.

Copy link
@uds5501

uds5501 Jun 20, 2019

Author Contributor

@shreyanshdwivedi OH! Okay, modifying the function!

This comment has been minimized.

Copy link
@uds5501

uds5501 Jun 20, 2019

Author Contributor

@shreyanshdwivedi @niranjan94 review now please

@uds5501 uds5501 force-pushed the uds5501:enh-2956 branch from 8e7f3b6 to f0a123a Jun 20, 2019

</button>
{{/if}}
</div>

This comment has been minimized.

Copy link
@Anupam-dagar

Anupam-dagar Jun 20, 2019

Member

A little less priority but I guess we can get rid of the extra line at Line 114 and 93.

@uds5501 uds5501 force-pushed the uds5501:enh-2956 branch from 4baf5eb to 35fc1c2 Jun 21, 2019

@uds5501

This comment has been minimized.

Copy link
Contributor Author

commented Jun 21, 2019

[]
);
} catch (error) {
this.notify.error(this.ln10.t(error.message));

This comment has been minimized.

Copy link
@mrsaicharan1

mrsaicharan1 Jun 21, 2019

Member

I guess this should be this.notify.error(this.l10n.t(error.message));.

This comment has been minimized.

Copy link
@uds5501

uds5501 Jun 21, 2019

Author Contributor

@mrsaicharan1 Thanks for pointing out the typo, I have fixed it now. Please review

@uds5501 uds5501 force-pushed the uds5501:enh-2956 branch from 35fc1c2 to 05ff8e3 Jun 21, 2019

@niranjan94 niranjan94 merged commit 3502065 into fossasia:development Jun 21, 2019

4 of 7 checks passed

Header rules No header rules processed
Details
Pages changed 6 new files uploaded
Details
Redirect rules No redirect rules processed
Details
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
Mixed content No mixed content detected
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
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.