-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
fix: Added quantity, min/max price validations & fixed donations bugs #3209
Conversation
6e43fcc
to
4bdc17b
Compare
10fefce
to
abe4651
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.
@mrsaicharan1 Please do the following else looks good
Whatever looks well can be done 👍 |
794bde1
to
f7ec5d5
Compare
@niranjan94 @CosmicCoder96 There was a bug related to the donation price. The subtotal was getting updated but the total wasn't as the price of the donation is variable. I fixed that by adding the ticket price to the watchable property |
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.
@mrsaicharan1 Please have a look here, Else is fine.
656563f
to
55876d4
Compare
I’ve written 2 validation rules checkMax and checkMin as multiple paramters are not supported by validation rules. It’s getting validated but it always returns false. Always prompts Ticket max price should be greater than min price |
@niranjan94 @CosmicCoder96 I had already written a validation check for min/max price on the server. I feel that the FE validation isn't required as Max/min tickets per order also follows the same practice. Please have a look here... |
6737ac4
to
566bf03
Compare
566bf03
to
5d5a5b4
Compare
@niranjan94 I've made the requested changes. Used |
app/components/public/ticket-list.js
Outdated
donationTicketsValidation: computed('donationTickets.@each.id', 'donationTickets.@each.minPrice', 'donationTickets.@each.maxPrice', function() { | ||
let validationRules = {}; | ||
this.donationTickets.forEach(donationTicket => { | ||
validationRules = merge(validationRules, { |
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.
Merge is not required for this. You're just creating a new object any way. You can just directly assign stuff with validationRules[donationTicket.id] =
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.
@niranjan94 Yeah, I guess you're right. Removed the redudant merge.
app/components/public/ticket-list.js
Outdated
@@ -176,14 +200,32 @@ export default Component.extend(FormMixin, { | |||
this.send('togglePromotionalCode', this.code); | |||
} | |||
}, | |||
donationTicketsValidation: computed('donationTickets.@each.id', 'donationTickets.@each.minPrice', 'donationTickets.@each.maxPrice', function() { | |||
let validationRules = {}; | |||
this.donationTickets.forEach(donationTicket => { |
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.
Use for-of loop
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.
@niranjan94 Done.
722bcf1
to
58f0a63
Compare
app/components/public/ticket-list.js
Outdated
donationTickets: computed.filterBy('data', 'type', 'donation'), | ||
|
||
isDonationPriceValid: computed('donationTickets.@each.orderQuantity', 'donationTickets.@each.price', function() { | ||
let returnValue = false; |
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.
Can be simplified into
for (const ticket of this.donationTickets) {
if (donationTicket.orderQuantity > 0) {
if (donationTicket.price < donationTicket.minPrice || donationTicket.price > donationTicket.maxPrice) {
return false
}
}
}
return true
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.
@niranjan94 Done.
Added watched properties and fixed totalling error Added semantic validation Added dynamic popup for ticket buyers Added action to check for max/min price Added quantitiy, min-max and bug fixes for donations removed repeated code Add error class validation Fixed paid & free tickets bug ESLint Used for-of & removed redundant merge Optimized donationsValidation property
ac5de5f
to
1f6c5dd
Compare
Fixes #3184
When organizer tries to add tickets worth of 0 value
![Screenshot 2019-06-24 at 11 45 37 PM](https://user-images.githubusercontent.com/25197147/60064287-8a33b100-971d-11e9-8d6a-0a4cd059d9f7.png)
Min max validation checks
![Screenshot 2019-07-02 at 9 00 44 AM](https://user-images.githubusercontent.com/25197147/60480703-29215580-9ca8-11e9-93da-651413a7b69a.png)
When the ticket buyer tries to add zero valued donation tickets
![Screenshot 2019-06-26 at 7 04 39 AM](https://user-images.githubusercontent.com/25197147/60144662-e9a6c500-97e0-11e9-8cb2-4493975d7fdd.png)
Short description of what this resolves:
Does not allow donation tickets worth of 0 value to be purchased & adds quantity support
Changes proposed in this pull request:
minPrice
andmaxPrice
)Checklist
development
branch.