-
Notifications
You must be signed in to change notification settings - Fork 20
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
Add GraphQL Operations for Event #66
Add GraphQL Operations for Event #66
Conversation
… and SearchEvents
name: { type: GraphQLString }, | ||
startTS: { type: GraphQLDateTime }, | ||
endTS: { type: GraphQLDateTime }, | ||
poster: { type: GraphQLID }, |
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.
poster of type GraphQLID?
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.
According to what we've planned, we'll be having a media collection, and we would just refer to the id of that media where ever required.
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.
That'll be a an ID in database but should resolve to poster and its blurhash here, isn't it?
endTS: { type: GraphQLDateTime }, | ||
poster: { type: GraphQLID }, | ||
type: { type: GraphQLInt }, | ||
host: { type: GraphQLID }, |
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.
Aren't we resolving the host from its ID?
createdAt: { type: GraphQLID }, | ||
createdBy: { type: GraphQLID }, | ||
updatedAt: { type: GraphQLDateTime }, | ||
updatedBy: { type: GraphQLID }, | ||
schemaVersion: { type: GraphQLInt }, |
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.
Are these five needed in GraphQL?
name: name, | ||
startTS: startTS, | ||
endTS: endTS, | ||
poster: poster, | ||
type: type, | ||
host: host, | ||
url: url, | ||
venue: venue, |
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.
Didn't this part give an ESLint warning?
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.
Also add schemaVersion, CAt, UAt, CBy, UBy
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.
Didn't this part give an ESLint warning?
What warning exactly?
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 format
{url:url}
can be reduced to
{url}
When key and value have same name
info, | ||
_EventModel = EventModel | ||
) => { | ||
try { |
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.
There should be a check to see if an event of that ID exists
Only Future events should be updated
listEvents: async (parent, { limit = DEF_LIMIT, offset = DEF_OFFSET }, context, info, _EventModel = EventModel) => { | ||
try { | ||
const _events = await _EventModel.find().skip(offset).limit(limit); | ||
return _events; | ||
} catch (e) { | ||
if (e instanceof GraphQLError) { | ||
return e; | ||
} | ||
return APIError(null, e); | ||
} | ||
}, | ||
}; |
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.
Only future events should be listed, or there should be a flag to show past events
The UI needs a list of events in time/date ordered, where is that handled?
type: new GraphQLNonNull(GraphQLString), | ||
}, | ||
}, | ||
resolve: searchEvents, |
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.
Is this resolver not yet done?
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.
Seach events isn't being done rn as @rutajdash told the approach we though of following was very expensive. After we decide on how to do it, it'll be done.
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.
Add a TODO or create an issue
Solved or Linked Issues
Fixes #59
Reference any and all issues solved and related to this pull request here. Mention if there are none.
Description and Changes
Your checklist for this pull request
🚨Please review the guidelines for contributing to this repository.
Post merge checklist