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

implement: questions endpoints #83

Merged
merged 8 commits into from
Dec 18, 2020
Merged

implement: questions endpoints #83

merged 8 commits into from
Dec 18, 2020

Conversation

NBNARADHYA
Copy link
Contributor

#60

@NBNARADHYA NBNARADHYA changed the title create: POST /questions route implement: questions endpoints Dec 15, 2020
cjchirag7
cjchirag7 reviewed Dec 15, 2020
View reviewed changes
Copy link
Collaborator

@cjchirag7 cjchirag7 left a comment

Choose a reason for hiding this comment

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

We probably also need to insert the creator of the question in questions_editors_map.

server/models/questions/createQuestion.js Outdated Show resolved Hide resolved
@ridhishjain-zepto
Copy link
Collaborator

ridhishjain-zepto commented Dec 15, 2020 •
edited
Loading

LGTM...

only one thing:
instead of adding tags in createQuestion API...we can have that in updateQuestion API...because it will already be there in updateQuestion. I guess the creator can add tags after creating a question...(after calling createQuestion API), i don't think we need to add tags in this one

thoughts @cjchirag7 ?

@NBNARADHYA
Copy link
Contributor Author

But..should'nt we be allowed to add tags to questions when creating them ? I guess updating it just for the sake of adding it would make frontend do two requests for that ? That would be slow I guess

@cjchirag7
Copy link
Collaborator

LGTM...

only one thing:
instead of adding tags in createQuestion API...we can have that in updateQuestion API...because it will already be there in updateQuestion. I guess the creator can add tags after creating a question...(after calling createQuestion API), i don't think we need to add tags in this one

thoughts @cjchirag7 ?

I feel it's okay for now, as creation of questions would be much less frequent, than updation.

cjchirag7
cjchirag7 requested changes Dec 15, 2020
View reviewed changes
server/models/questions/getQuestion.js Outdated Show resolved Hide resolved
@NBNARADHYA
Copy link
Contributor Author

LGTM...

only one thing:
instead of adding tags in createQuestion API...we can have that in updateQuestion API...because it will already be there in updateQuestion. I guess the creator can add tags after creating a question...(after calling createQuestion API), i don't think we need to add tags in this one

thoughts @cjchirag7 ?

BTW we are inserting into tags table only if tags is provided. So it is optional

cjchirag7
cjchirag7 approved these changes Dec 16, 2020
View reviewed changes
Copy link
Collaborator

@cjchirag7 cjchirag7 left a comment

Choose a reason for hiding this comment

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

LGTM

@NBNARADHYA NBNARADHYA requested a review from cjchirag7 December 16, 2020 11:32
cjchirag7
cjchirag7 requested changes Dec 16, 2020
View reviewed changes
Copy link
Collaborator

@cjchirag7 cjchirag7 left a comment

Choose a reason for hiding this comment

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

Everything looks fine, but we need to add search by tag also.

server/models/questions/getPublicContestQuestions.js Show resolved Hide resolved
server/models/questions/getPublicContestQuestions.js Show resolved Hide resolved
ridhishjain-zepto
ridhishjain-zepto requested changes Dec 16, 2020
View reviewed changes
server/routes/index.js Outdated Show resolved Hide resolved
server/routes/questions/getEditorQuestions.js Outdated Show resolved Hide resolved
cjchirag7
cjchirag7 approved these changes Dec 17, 2020
View reviewed changes
Copy link
Collaborator

@cjchirag7 cjchirag7 left a comment

Choose a reason for hiding this comment

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

LGTM

@cjchirag7
Copy link
Collaborator

@ridhishjain , should we merge now ?

@NBNARADHYA
Copy link
Contributor Author

@cjchirag7 Wait for other commits related to questions endpoint..I will push them and then it can be merged at once

@ridhishjain-zepto
Copy link
Collaborator

LGTM till now

ridhishjain-zepto
ridhishjain-zepto requested changes Dec 18, 2020
View reviewed changes
server/models/questions/addEditor.js Outdated Show resolved Hide resolved
server/models/questions/forkQuestion.js Outdated Show resolved Hide resolved
server/models/questions/forkQuestion.js Show resolved Hide resolved
server/models/questions/forkQuestion.js Show resolved Hide resolved
server/models/questions/removeEditor.js Outdated Show resolved Hide resolved
server/schema/questions/index.js Show resolved Hide resolved
server/schema/questions/addEditor.js Outdated Show resolved Hide resolved
server/routes/questions/removeEditor.js Show resolved Hide resolved
server/routes/questions/addEditor.js Show resolved Hide resolved
cjchirag7
cjchirag7 requested changes Dec 18, 2020
View reviewed changes
Copy link
Collaborator

@cjchirag7 cjchirag7 left a comment

Choose a reason for hiding this comment

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

A few changes required. Rest looks great!

server/models/questions/forkQuestion.js Outdated Show resolved Hide resolved
server/models/questions/removeEditor.js Outdated Show resolved Hide resolved
server/models/questions/updateQuestion.js Outdated Show resolved Hide resolved
server/routes/questions/addEditor.js Show resolved Hide resolved
server/schema/questions/index.js Show resolved Hide resolved
cjchirag7
cjchirag7 approved these changes Dec 18, 2020
View reviewed changes
Copy link
Collaborator

@cjchirag7 cjchirag7 left a comment

Choose a reason for hiding this comment

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

LGTM!

@cjchirag7
Copy link
Collaborator

@ridhishjain , you may also review once. So, that we may merge it, if it's fine.

@ridhishjain-zepto ridhishjain-zepto merged commit 2960cbc into Cyber-Labs:master Dec 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjchirag7 cjchirag7 cjchirag7 approved these changes

@ridhishjain-zepto ridhishjain-zepto ridhishjain-zepto approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@NBNARADHYA @ridhishjain-zepto @cjchirag7