-
Notifications
You must be signed in to change notification settings - Fork 80
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: Issue24 registration api backend #26
feat: Issue24 registration api backend #26
Conversation
410cdfe
to
f498779
Compare
update: https://bridge-in-tech-bit-test.herokuapp.com Important: Currently the db has mock data populated using As for the MS API Heroku that is connected to MS db (only has MS schema), below is the link for it https://bridge-in-tech-ms-test.herokuapp.com/ The ms heroku link above is only going to be used as the |
d058eda
to
da89ef2
Compare
Also @ramitsawhney27 , for Wrong password and/or username, MS is returning 404 instead of 401. I put 401 in BIT. But should I open an issue on MS backend so someone can change it there? |
da89ef2
to
7f8f817
Compare
Update: |
Check out the comment on this - #15 |
7f8f817
to
42f7bbb
Compare
PS: please note that the recent code change is including .travis.yml and deploy.sh files |
Maya can you please move Travis changes to its own issue? |
f33fd4f
to
616bd93
Compare
9eb42f3
to
7602e04
Compare
@mtreacy002 do you understand this change regarding comment #26 (comment)? @ramitsawhney27 For the sake of time and planning, could we have this change as technical debt, as an issue to be taken up later? either my Maya or any other contributor. I'm saying this because of the planning set out for Maya for GSoC. There were some changes here that we didn't predict with baggage coming from MS code, so I see no problem with leaving some changes for other issues. |
@mtreacy002 Could you kindly resolve all the comments above that are not relevant anymore? This helps in other people go through and review the PR smoothly. |
@SanketDG Are you sure I'm allowed to do that (mark comments as resolved)? Wouldn't the mentor need to give their approval and state it as resolve? I mean, I've done the changes but the mentor needs to confirm they are happy with the changes I've made. cc @ramitsawhney27 |
7602e04
to
c8a10a5
Compare
Update @anitab-org/bridgeintech-maintainers . I've fixed the http error responses: Note that for error 409: Conflict, as the PR#620 on mentorship-backend that will fix the error code returned is still in process, atm it is still returned as 400: Bad request |
* Add the README file * made changes to README * added suggested changes * docs: Move lines anitab-org#8 and anitab-org#10 * moved README outside .github * remove extra README file
c8a10a5
to
4967740
Compare
I have a couple of points:
Since, I have a lot of basic questions, I feel I need to get up to speed on them before I add in my approval. However, @foongminwong if you feel all looks good, you can merge. I can catch up meanwhile. |
Hi @meenakshi-dhanani . Thanks for sharing your views. I'll try to answer them the best I can. But since I'm also a newbie and have no real-world exposure to the common/best practice, @ramitsawhney27 probably will be able to give you better advice on things related to db questions).
Yup, agree, going to look at that on a separate issue
will look around for the options. Can you also help give me the link if you find one? Ta
I understand your point. With my limited knowledge (only based on what I've learned so far from uni/self-learning), there are few things I consider when working with database:
I'll try to answer the last 2 questions in one go:
First of all, the reason why we have 2 schemas MS and BIT is because we want the two system to be loosely coupled (able to run independently to some extent) in particular since MS is so close to production. Changing the database directly inside MS schema and white-label the project to adopt the features of BIT is not an ideal option at this time. As discussed at the planning stage with issue#10, at the end of GSoC, what we're trying to achieve is having 2 APIs (BIT+MS) and one database (still with 2 schemas). However, with the challenges stated in my above answer (not easy to add columns/tables on the fly when persistent data involved) we need to take extra care not to disrupt MS development. The unavoidable dependent of BIT towards MS development happens since the For the BIT MS to be loosely coupled with the 2 APIs and one db scenario we're trying to achieve, MS code base also needs to be aware that BIT schema exist (this means adding BIT models inside MS repo) although the interactions between the 2 only will be done through sending request to each other's API. Again, this unlikely to happen immediately without disrupting MS development or GSoC schedule (aka cross-project issues - needs to open issues on MS repo for someone to work on this BIT schema injection and finally, MS database migration to become one db with BIT) That's also why, in the initial stage of GSoC, we'll make do with having 2 db's and later/gradually during GSoC, we'll look at how we're going to tackle this (merge the dbs). Even though it means with Register atm we only saving data in MS db, not on BIT db, but the 2 schemas need to be there at the beginning since it is the db design we want to achieve and with design on persistent data, we aim to follow "waterfall" principal rather than "agile" so not to make too many changes on the design which will make data management messy. Hopefully this helps answer your questions. Again, I have no experience in this matter and only based my opinion from uni and self-learning. But @ramitsawhney27 probably can share his point of view on this too. 😉 Hope this answers |
The changes made in this PR were tested locally. Following are the results:
{
"name": "vibere",
"username": "vibere",
"password": "123456789",
"email": "vibere6497@psk3n.com",
"terms_and_conditions_checked": true,
"need_mentoring": false,
"available_to_mentor": true,
"is_organization_rep": true,
"timezone": "CENTRAL TIME"
}
|
About this point: Unlike the belief or what is taught in uni, we can create the database on the fly. We do that with incremental migrations. This article will help think in that manner - https://martinfowler.com/articles/evodb.html |
Hi @foongminwong , just wondering. perhaps you've pulled code was outdated and you need to pull the latest code from the PR? The latest code in the PR should have the similar fields on Swagger UI to the one for registration on MS API |
@mtreacy002 so about the 2 dbs, I get it as a temporary solution for maybe GSoC. So for this story, I tested with sending a POST request to create user. Should there be an entry in the MS tables for a user at least? I am getting the correct responses from the API, I just want to validate where the record is stored. |
@meenakshi-dhanani. You can check it by querying directly to the MS db you run in parallel with the BIT API when you did the test (I ran my own MS API on local port 4000). You can do this either on psql shell or pgadmin4 whichever one you prefer. |
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 changes made in this PR were tested locally again. Following are the results:
-
Code review - Done (see previous code reviews from other mentors)
-
All possible responses were tested as below:
Expected Result: As a user, I can register under BIT.
Actual Result: Same as expected. After registering, the user will receive an automatic email sent by MS for account verification. The user is also able to login into MS with the created account credentials. -
Additional testcases covered:
Expected Result: It should throw an error
A user with that username already exists.
Actual Result: Same as expected/- Register a user with invalid password (password length less than 7 characters) on BIT
Screenshot/gif/url:
Expected Result: It should throw an error
The password field has to be longer than 7 characters and shorter than 65 characters.
Actual Result: Same as expected.Expected Result: As a user, I can login into MS using the account credentials created in BIT.
Actual Result: Same as expected. - Register a user with invalid password (password length less than 7 characters) on BIT
-
Additional Comments:
- Ready to Merge ✔ @anitab-org/bridgeintech-maintainers ?
-
Input for registration testing:
{
"name": "vibere",
"username": "vibere",
"password": "123456789",
"email": "vibere6497@psk3n.com",
"terms_and_conditions_checked": true,
"need_mentoring": true,
"available_to_mentor": true
}
-
Status of PR Changed to: N/A
-
OS Version: Windows 10
@meenakshi-dhanani - do you have any additional comments, or are we good to merge? |
Thank you @foongminwong for your detailed tests. And congratulations on your successful setup 🎉 |
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.
Had a call with Maya to validate if the call to API is adding the records in the MS database. It works.
Description
Create BIT API Users service with Register functionality so that a new user can register to BIT.
Fixes #24 and replacement of PR #25
NOTE: As this PR also carries code changes on PR #25, the changes needed in related to PR#25 is only made to this PR (#26). The PR#25 is to be closed without merging.
Type of Change:
Code/Quality Assurance Only
How Has This Been Tested?
Tested sending POST /register request to MS API with successful response.
TO DO:
Checklist:
Code/Quality Assurance Only