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.

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

Badge GET api #163

Merged
merged 8 commits into from Mar 6, 2021
Merged

Badge GET api #163

merged 8 commits into from Mar 6, 2021

Conversation

whyDontI
Copy link
Contributor

@whyDontI whyDontI commented Feb 8, 2021

Added

  • GET /badges

API Contracts

Real-Dev-Squad/website-api-contracts#29

@whyDontI whyDontI self-assigned this Feb 8, 2021
@ankushdharkar ankushdharkar temporarily deployed to backend-rds-pr-163 February 8, 2021 11:15 Inactive
Copy link
Contributor

@Cartmanishere Cartmanishere left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@Rucha1499 Rucha1499 left a comment

Choose a reason for hiding this comment

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

Great work!! @whyDontI 🎉 😄

Can we generate the API Schema for this route?

@ankushdharkar ankushdharkar temporarily deployed to backend-rds-pr-163 March 2, 2021 17:44 Inactive
@whyDontI whyDontI requested a review from Rucha1499 March 2, 2021 17:45
@whyDontI whyDontI changed the title [WIP] Badge GET api Badge GET api Mar 2, 2021
Rucha1499
Rucha1499 previously approved these changes Mar 2, 2021
Copy link
Contributor

@Rucha1499 Rucha1499 left a comment

Choose a reason for hiding this comment

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

Looks great!! 🎉

Copy link
Contributor

@swarajpure swarajpure left a comment

Choose a reason for hiding this comment

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

Having a doubt in the test case 😅

Comment on lines +9 to +26
describe('Badges', function () {
describe('GET /badges', function () {
it('Should get all the list of badges', function (done) {
chai
.request(app)
.get('/badges')
.end((err, res) => {
if (err) { return done() }
expect(res).to.have.status(200)
expect(res.body).to.be.a('object')
expect(res.body.message).to.equal('Badges returned successfully!')
expect(res.body.badges).to.be.a('array')

return done()
})
})
})
})
Copy link
Contributor

Choose a reason for hiding this comment

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

Where are we adding the badges in the database to GET them? 🤔
How will this test case pass on CI? 🤔

Copy link
Contributor Author

@whyDontI whyDontI Mar 2, 2021

Choose a reason for hiding this comment

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

Ankush is going to add badges manually in database until we get the authorization in place.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ankush is going to add badges manually in database until we get the authorization in place.

I guess the CI also uses firebase emulators that's why we install it on every commit.
I meant the CI doesn't use the production db, right?
Feel free to correct me 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, seems like CI uses emulator, check test.yml file in .git/workflow
CI fetching and adding data in prod DB won't be a good idea

Copy link
Contributor

Choose a reason for hiding this comment

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

That's what I'm saying 😅
The prod DB will have the badges details
but the firebase emulator wont and hence the test wouldn't pass 😅

Let me know if I'm wrong 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, that's the convention, even if the GET Endpoint returns an empty array, the response code will still be 200, and the request is considered fulfilled.

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool

swarajpure
swarajpure previously approved these changes Mar 6, 2021
Copy link
Contributor

@swarajpure swarajpure left a comment

Choose a reason for hiding this comment

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

Looks good! 👌🏻

@whyDontI whyDontI dismissed stale reviews from swarajpure and Rucha1499 via a66d0a6 March 6, 2021 06:08
@ankushdharkar ankushdharkar temporarily deployed to backend-rds-pr-163 March 6, 2021 06:08 Inactive
@swarajpure swarajpure merged commit 462760b into develop Mar 6, 2021
@swarajpure swarajpure deleted the feature/badges branch March 6, 2021 07:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants