-
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: Add post organization/<int:organization_id>/programs/<int:program_id>/send_request endpoint #185
base: develop
Are you sure you want to change the base?
Conversation
Hello there!👋 Welcome to the project!💖 Thank you and congrats🎉 for opening your first pull request.✨ AnitaB.org is an inclusive community, committed to creating a safe and positive environment.🌸Please adhere to our Code of Conduct and Contribution Guidelines.🙌.We will get back to you as soon as we can.😄 Feel free to join us on AnitaB.org Open Source Zulip Community.💖 We have different streams for each active repository for discussions.✨ Hope you have a great time there!😄 |
@mtreacy002 Please have a look , Thank you! |
I just want to clarify something with @isabelcosta or other @anitab-org/admins . During GSoC, any new code must be accompanied by writing test cases for the same functionality. Is this still the case, @admins? If so, can it be done on 2 separate issues or all in one issue (which means this PR should be added with test cases)? |
@mtreacy002 I would like to complete this even the testcases . Can i start working on that ? However I need 2-3 days for this . |
The concern to put these 2 issues (code change and test cases) are that it makes it harder for the reviewer and contributor and the process will take longer. Normally it’s ok when there aren’t many files being changed. But this functionality in particular is a big one (which is understandable if you need more time to work on 😁), so I personally prefer to have the test cases written on a different issue. But we’ll have to see what the admin think. In the meantime, I can start reviewing this as soon as I am free (working on uni assignment atm |
Usually, when coding a feature we should code tests with it. we can catch bugs by writing those and also, in the future if someone writing another feature breaks the one from this PR, the tests will detect that. It's a best practice we try to follow. Also the best person to write initial tests for a feature is the person who wrote the feature. So best option here is to write tests along with this feature implementation. @mtreacy002 |
Thanks @isabelcosta for your clarification. @decon-harsh , I think you can go ahead and add the test cases then, please. That's ok if it takes another 2-3 days (even longer if needed) for you to work on it. Just keep us updated 😉 |
@mtreacy002 already started 😃 |
8eb1578
to
fe9d46f
Compare
@mtreacy002 Added new tests, I am not much confident on the tests, So I request the review team for a good check and let me know whether this is right or wrong. Will add more test cases then. Also, I suggest using selenium testing method (Used in VMS) for better testing, It will be useful for frontend as well as backend |
I don't know much about selenium, isn't that for frontends? You can always create an issue and there can be more discussion around this. @decon-harsh |
Yes , but i guess we can use it to send datas in forms and as a result we can test frontend as well as backend . I might be wrong will check it's implementation in VMS and will ask about this in upcoming BIT Sessions as well. |
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.
@decon-harsh , I've only done some of the review on the code you've written here. But there is a critical point that I need you to rethink on the approach of the issue. Please check my comments below.
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.
Just additional feedback.
@decon-harsh, I think we might need to create a different request body for the BIT app/api/model/mentorship_relation. This is because the request is not actually done between mentee and mentor but between mentee/mentor and organization representative. So, we need to create the request body that can show this (just like what's needed for creating the mentorship relation as per example on the db_add_mock.py on BIT initial test below - note that creation_date and status will not be needed in the request body model and will be filled by the logic separately). The problem is, how we're going to access the db, since Mentorship Relation is an entity within MS schema.
@isabelcosta and @ramitsawhney27, can I please ask your opinion on how best to approach this? Thanks |
Hey @mtreacy002 so sorry I wasn't able to reply to you , I am currently preoccupied with my personal urgent works . I will check all the message and do all the necessary changes after few hours . Pardon me please! |
Sure, @decon-harsh. Don’t worry, you don’t need to respond immediately 😂. Just respond whenever you have time 😉 |
@mtreacy002 Thank you for being very patient and helpful. Easy changes are done for now! Please have a look.
|
I didn't get this,
You had mentioned it should be both ways. In this we are directly making an API call to the MS backend (Line 145 in resources/mentorship_relation.py) to create a mentee - mentor relation b/w org_rep and mentee, and if 201 CREATED querying that relation_id to extend it. |
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.
@mtreacy002 Thank you for being very patient and helpful. Easy changes are done for now! Please have a look.
Yeah, I copied the code skeleton from MS backend resources/mentorship_relation.py forgot the new formatting.
- Was the code fine, you mentioned something related to the internal API call. Am I doing it right?
No, I was the one who had missed the line for the call to ms api, that's why I deleted that comment when I realised you've used it, sorry, my bad 😊
mentorship_relation_data['end_date'] = data['end_date'] | ||
mentorship_relation_data['notes'] = data['notes'] | ||
|
||
response = http_response_checker(post_request_with_token("/mentorship_relation/send_request",token, mentorship_relation_data)) |
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.
I noticed that you used the internal call here, that's good.
send_mentorship_extension_request_body = Model( | ||
"Send mentorship relation request to organziation model", | ||
{ | ||
"mentee_id": fields.Integer( |
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.
I'm not sure if we should use the same model as Mentorship relation request. We could create our own model for BIT by using the request body with action_user_id with value being organization representative_id.
@decon-harsh, yes, I did mentioned it has to be both ways (mentor/mentee can initiate request to a program and a program can initiate request to mentor/mentee) but the relationship creation will unfold through different execution times as what would happen in real job application/job offer, so not through a one time event. To be exact the flows I have in mind are something like these:
From the gist examples above, at the end the mentor and mentee are connected in the mentorship relation and to the program which enable them to work on tasks and share notes when the program starts. Back to my point, if we are to follow the flows illustrated on my gists example, we need to create an api service with DAO class having functionality/ies to perform these actions. The question is, which side we should create this api on, MS or BIT. |
Yeah, my mistake I fixed Mentor to be org.rep_id. The 1st approach would be fine , and I will be happy to solve this cross project issue at MS Backend if it's get approved. I will devise the procedure and flow and will share to you in a day or two. Thank you for clarifying my doubts. |
Sorry, I mixed up the gist examples on the comment above, so I've updated the titles according to the relevant gist flows |
Update, @decon-harsh . I don't think we need to open a cross project issue on MS Backend anymore. Coz we can just add the DAO directly to BIT version of MS backend server - the one from my fork repo (just like we did with CORS and postgres on MS backend). We just need to wait for advice from @isabelcosta and @ramitsawhney27 to see if this is the case. |
Yeah and till then I will note down what and how I will implement these. Will keep you updated @mtreacy002 |
@mtreacy002 Any updates on this. Creating DAO on your version of MS Backend got approved? |
@decon-harsh , I've reposted the request for their input on Zulip. cc @isabelcosta and @ramitsawhney27. |
In the meantime, have you created a gist to show your approach on this MentorshipRelationModel manipulation, @decon-harsh ? With the gist, you just need to show how the events flow you're proposing will affect the MentorshipRelationModel and MentorshipRelationExtensionModel without having to worry about which side of the backend server the DAO is going to be placed. We can discuss this while waiting for the input from @isabelcosta or @ramitsawhney27. |
@mtreacy002 Can we do this?
|
@decon-harsh , I actually thought that this would be a good alternative to have bridging table for mentor/mentee user and organisation (through program - so, maybe call it MentoringProgramRelationModel). However, I am concerned that it would introduce 2 types of redundancies:
But again, I agree that this could save us from the trouble of placing DAO on MS side (not ideal for MS as it is only specific to BIT purpose) or BIT side (not ideal as it would breach the integration flow as BIT by design currently has to make API requests for anything that would lead to MS database models modification). So, we have to decide which one is the lesser evil here 🤣🤔. |
One more thing @mtreacy002 , While I was going through the gists you provided , i came across this problem . There is a problem of state in that approach When a program sends a request , state is Pending Suppose you want to know whether , mentor accepted your request or not , then you have to check the action_user_id but in the second scenario the action_user_id is just opposite of first. |
Yes data redundancy is a problem , but here in this approach we can easily check states and manage Mentor Mentee. Also the actual Mentorship Relationship won't be created till a program gets both mentor and mentee . We will make internal api call to ms back-end when the user accepts the request (click the link of confirmation in email). |
@decon-harsh , can you specify the exact gist and lines where you suspect this happens so I can check it out, thanks |
@decon-harsh, I agree, this is the advantage of using this approach.
can you elaborate how this internal call is going to be made? I'm assuming we utilise existing MS api endpoints ("mentorship_relation/send_request", "mentorship_relation/int:request_id/(accept/reject)"), with the current flow to create mentorship relation on MS, right? This is what I referred to in point 2> Duplication of Relationship requests process. |
@decon-harsh , to be honest, although what your suggesting involves redundancies in database and actions, I think it is a much cleaner approach and can help draw a clear line between BIT role (create Mentoring Program Relation) and MS role (create/manage Mentorship relation). |
@mtreacy002 If I have to choose the creating the DAO option on your repo I will exactly follow your gist. |
Description
Added an endpoint POST organization/int:organization_id/programs/int:program_id/send_request which creates mentor-mentee relation between two users and extends the relation with program of an organization mentor is representative/owner.
Merge this after anitab-org/mentorship-backend#994 which doesn't let two users to create same relations multiple time.
Fixes #161
Type of Change:
Code/Quality Assurance Only
How Has This Been Tested?
Manually using postman
Wrote new tests
Checklist:
Code/Quality Assurance Only