-
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
feat: Rename Fees to Revenue and add Event Date in Admin Sales Fees. #3211
Conversation
|
||
export default Route.extend({ | ||
titleToken() { | ||
return this.l10n.t('Fees'); | ||
return this.l10n.t('Revenue'); |
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.
The route name also should chan ge
app/routes/admin/sales/fees.js
Outdated
}, | ||
|
||
model() { | ||
return this.store.findAll('admin-sales-fee'); | ||
return RSVP.hash({ |
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 async-await instead of RSVP.hash
app/routes/admin/sales/fees.js
Outdated
}, | ||
|
||
afterModel(model) { | ||
model.orders.forEach((data, index) => data.set('eventDate', model.events[index].startsAtDate)); |
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.
Do this in model
app/routes/admin/sales/fees.js
Outdated
model.orders.forEach((data, index) => data.set('eventDate', model.events[index].startsAtDate)); | ||
}, | ||
|
||
setupController(controller, model) { |
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.
Do not do this.
@niranjan94 This is ready for another review. |
app/routes/admin/sales/revenue.js
Outdated
async model() { | ||
let orders = await this.store.findAll('admin-sales-fee', { reload: true }).then(orders => orders.sortBy('name')); | ||
let events = await this.store.findAll('event', { reload: true }).then(events => events.sortBy('name')); | ||
orders.forEach((data, index) => data.set('eventDate', events[index].startsAtDate)); |
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.
Please do not assume the indexes of both orders and events match. (ie) for example, do not assume orders[0]
matches with events[0]
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 actually assumed it on the basis that I sorted them on the basis of name. I guess name in orders and events are event name only, right? I wasn't able to find another way to get event date as their is no relation to event in orders model. Maybe we can discuss this in tomorrow's meeting.
This PR has now a dependency on corresponding server PR fossasia/open-event-server#6135 |
app/routes/admin/sales/revenue.js
Outdated
}, | ||
|
||
model() { | ||
async model() { | ||
return this.store.findAll('admin-sales-fee'); |
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.
not required here, if you are not using await.
app/routes/admin/sales/revenue.js
Outdated
}, | ||
|
||
model() { | ||
async model() { |
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.
async not needed
app/models/admin-sales-fee.js
Outdated
@@ -1,11 +1,13 @@ | |||
import attr from 'ember-data/attr'; | |||
import ModelBase from 'open-event-frontend/models/base'; | |||
import moment from 'moment'; |
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.
Never used. Please remove this as travis is failing due to this
This PR is ready for another review. |
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.
Has the field already been added on server?
@CosmicCoder96 yes the server PR is merged. |
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.
Looks good.
can we merge this? |
Reference: #3173
Short description of what this resolves:
Currently there is no event date in the table of fees in admin sales section. This PR adds an event date to the table of fees in admin sales section. Also the Fees is renamed to Revenue.
Changes proposed in this pull request:
Checklist
development
branch.Screenshots