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.

Sign up for GitHub

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

Jump to bottom

Refactor to display events on public page via New API #399

Merged
merged 2 commits into from
Jul 3, 2017
Merged

Refactor to display events on public page via New API #399

merged 2 commits into from
Jul 3, 2017

Conversation

abhinavk96
Copy link
Contributor

@abhinavk96 abhinavk96 commented Jul 2, 2017 •
edited
Loading

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)

Short description of what this resolves:

Refactors the models and app to display events on public page via new API.

Changes proposed in this pull request:

  • Lock ember data to 2.13.2
  • temporarily disable speakersCall
  • Refactor to display events on public page via new API

image

@abhinavk96 abhinavk96 self-assigned this Jul 2, 2017
@fossasia fossasia deleted a comment from open-event-bot bot Jul 2, 2017
@fossasia fossasia deleted a comment from open-event-bot bot Jul 2, 2017
niranjan94
niranjan94 suggested changes Jul 3, 2017
View reviewed changes
Copy link
Member

@niranjan94 niranjan94 left a comment

Choose a reason for hiding this comment

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

@CosmicCoder96

app/models/event.js Outdated
@@ -100,14 +100,14 @@ export default Model.extend({
*
* @see app/models/discount-code.js
*/
discountCode: belongsTo('discount-code'),
discountCode: belongsTo('discount-code', {inverse:null}),
Copy link
Member

Choose a reason for hiding this comment

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

Lets keep event model clean and specify inverses on the discount-code model instead. Also, specify the correct inverses. Not null.

  event   : belongsTo('event', {
    inverse: 'discountCodes'
  }), // The event that this discount code belongs to [Form (2)]
  events: hasMany('event', {
    inverse: 'discountCode'
  })    // The events that this discount code has been applied to [Form (1)]

app/routes/index.js Outdated
filter:[
{
name: 'starts_at',
op:'ge',val:moment().toISOString()
Copy link
Member

Choose a reason for hiding this comment

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

move val to the next line please

app/serializers/event-topic.js Outdated
@@ -2,6 +2,10 @@ import ApplicationSerializer from 'open-event-frontend/serializers/application';

export default ApplicationSerializer.extend({
attrs: {
subTopics: 'eventSubTopics'
type : 'event-type',
Copy link
Member

Choose a reason for hiding this comment

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

This is the event topic serializer. Should only have subTopics: 'event-sub-topics'

niranjan94
niranjan94 suggested changes Jul 3, 2017
View reviewed changes
Copy link
Member

@niranjan94 niranjan94 left a comment

Choose a reason for hiding this comment

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

@CosmicCoder96 something seems to be wrong the with the updated lockfile. See travis logs.

@abhinavk96 abhinavk96 force-pushed the backup branch 3 times, most recently from df44f1c to 09b53df Compare July 3, 2017 16:09
@fossasia fossasia deleted a comment from open-event-bot bot Jul 3, 2017
@fossasia fossasia deleted a comment from open-event-bot bot Jul 3, 2017
@fossasia fossasia deleted a comment from open-event-bot bot Jul 3, 2017
niranjan94
niranjan94 suggested changes Jul 3, 2017
View reviewed changes
Copy link
Member

@niranjan94 niranjan94 left a comment

Choose a reason for hiding this comment

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

@CosmicCoder96 what have you done here ? Please ... don't update any other packages. And don't use shrinkwrap. Let npm v5 generate it's own lockfile.

@niranjan94 niranjan94 force-pushed the fix/json-api-compat branch from 929c22c to 4ede73c Compare July 3, 2017 16:19
@niranjan94
Copy link
Member

Also @CosmicCoder96 the base branch was rebased. So, please update your branch accordingly.

@fossasia fossasia deleted a comment from open-event-bot bot Jul 3, 2017
@abhinavk96
Copy link
Contributor Author

@niranjan94 It was a temporary mistake, didn't knew base branch was rebased, have done it now.

@niranjan94
Copy link
Member

@niranjan94 It was a temporary mistake, didn't knew base branch was rebased, have done it now.

@CosmicCoder96 that's alright... I rebased it just now. That's why I added a comment :)

@dilpreetsio
Copy link
Member

dilpreetsio commented Jul 3, 2017 •
edited
Loading

@CosmicCoder96 @niranjan94 Why are we locking ember-data to 2.13.2 when we have 2.14.3?

niranjan94
niranjan94 suggested changes Jul 3, 2017
View reviewed changes
Copy link
Member

@niranjan94 niranjan94 left a comment

Choose a reason for hiding this comment

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

@CosmicCoder96 run eslint please and ensure there are no linting errors.

app/serializers/event-topic.js Outdated
@@ -2,6 +2,6 @@ import ApplicationSerializer from 'open-event-frontend/serializers/application';

export default ApplicationSerializer.extend({
attrs: {
subTopics: 'eventSubTopics'
subTopics : 'event-sub-topics'
Copy link
Member

Choose a reason for hiding this comment

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

Would cause an eslint fail.

app/routes/index.js Outdated
include: 'event_topic,event_sub_topic,event_type',
filter:[
{
name: 'starts_at',
Copy link
Member

Choose a reason for hiding this comment

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

The colons are not aligned. Would cause an eslint fail.

@niranjan94
Copy link
Member

@geekyd we have a bug in the latest ember-data package with regards to displaying model relationships on the template. So, until they fix it, we lock our package to the version that works.

In js, it's always better to lock your packages and update after testing.

@niranjan94
Copy link
Member

@geekyd -> emberjs/data#5023

@fossasia fossasia deleted a comment from open-event-bot bot Jul 3, 2017
@dilpreetsio
Copy link
Member

@niranjan94 Got it. Thanks for the reference :)

@fossasia fossasia deleted a comment from open-event-bot bot Jul 3, 2017
@abhinavk96
Copy link
Contributor Author

abhinavk96 commented Jul 3, 2017 •
edited
Loading

@niranjan94 @geekyd fixed all of linting

@fossasia fossasia deleted a comment from open-event-bot bot Jul 3, 2017
niranjan94
niranjan94 suggested changes Jul 3, 2017
View reviewed changes
app/templates/components/event-card.hbs Outdated
</span>
</div>
{{#smart-overflow class='description'}}
{{event.shortLocationName}}
Copy link
Member

Choose a reason for hiding this comment

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

Why have you used description instead of shortLocationName ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@niranjan94 Had confused it with the class thought it should be here 😅 apologies, I have changed it back.

@fossasia fossasia deleted a comment from open-event-bot bot Jul 3, 2017
@niranjan94 niranjan94 merged commit d9e5369 into fossasia:fix/json-api-compat Jul 3, 2017
@mmun mmun mentioned this pull request Aug 10, 2017
Merged
7 tasks
@mmun
Copy link

mmun commented Aug 10, 2017

@niranjan94 We've released a new version of ember data that may fix the issue you had.

@niranjan94
Copy link
Member

@mmun thanks for letting us know :D ... Yep looks like it fixes the issue #654 ... 🎉

@abhinavk96 abhinavk96 deleted the backup branch September 3, 2017 06:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@niranjan94 niranjan94 niranjan94 approved these changes

Assignees

@abhinavk96 abhinavk96

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@abhinavk96 @niranjan94 @dilpreetsio @mmun