Feat: Add BIT personal background to MS schema #1092

mtreacy002 · 2021-05-03T01:48:25Z

Description

Added BIT Personal Background to MS schema

Fixes #1091

Type of Change:

Code

Code/Quality Assurance Only

New feature (non-breaking change which adds functionality pre-approved by mentors)

How Has This Been Tested?

run existing tests and confirmed all tests passed.

run migration script (to create new alembic version for updated schema) successfully.

Checklist:

My PR follows the style guidelines of this project
I have performed a self-review of my own code or materials
I have commented my code or provided relevant documentation, particularly in hard-to-understand areas

Code/Quality Assurance Only

My changes generate no new warnings
New and existing unit tests pass locally with my changes

Add new migration script reformat to black linting

codecov · 2021-05-03T01:58:57Z

Codecov Report

Merging #1092 (f8c0c12) into bit (f25a0b8) will decrease coverage by 0.26%.
The diff coverage is 90.62%.

@@            Coverage Diff             @@
##              bit    #1092      +/-   ##
==========================================
- Coverage   94.50%   94.24%   -0.27%     
==========================================
  Files          39       40       +1     
  Lines        2677     2868     +191     
==========================================
+ Hits         2530     2703     +173     
- Misses        147      165      +18

Impacted Files	Coverage Δ
app/database/models/personal_background.py	`90.19% <90.19%> (ø)`
app/utils/bitschema_utils.py	`98.20% <90.55%> (-1.64%)`	⬇️
run.py	`94.87% <92.85%> (-1.29%)`	⬇️

mtreacy002 · 2021-05-03T02:07:14Z

@isabelcosta and @epicadk .The codecov %-tages are dropping since I didn't add a test_database_models for personal background. Do you want me to add the test on this PR or add later in a separate one?

epicadk · 2021-05-03T03:51:30Z

app/database/models/personal_background.py

@@ -0,0 +1,146 @@
+from sqlalchemy import null


Can you add a comment here specifying that this is a BIT extension?

epicadk · 2021-05-03T03:56:28Z

run.py

    db.create_all()

+    @application.shell_context_processor
+    def make_shell_context():
+        return {


Why is this required?

@epicadk , tbh this is my first attempt doing migration. I'm using the shell_context_processor based from what I gathered here. Please do let me know how to improve this 😉 .

epicadk · 2021-05-03T03:56:36Z

app/utils/bitschema_utils.py

+@unique
+class SexualOrientation(Enum):
+    HETEROSEXUAL = "Heterosexual/Straight"
+    LGBTIA = "LGBTIA+"


I think you forgot the Q LGBTQIA ?

will make the change soon 😉

epicadk · 2021-05-03T03:58:10Z

app/database/models/personal_background.py

+    years_of_experience = db.Column(db.Enum(YearsOfExperience))
+    others = db.Column(JsonCustomType)
+    is_public = db.Column(db.Boolean)
+


Not sure why exactly we would need such information. I'll leave that to @isabelcosta

@epicadk , do you mean years of experience? First of all, I believe this is relevant to support one of BIT goals which is to improve equity in IT because many with no to limited years of experience in IT were left out from employment opportunity. We often see a position for entry level required more than 3 - 5 years of experience. Stating this years of experience encouraging industries to target people within the lower years of experience to be the mentee and people with the higher level of experience to be the mentor. Their willingness to go against the norm in employment market (aka to be more inclusive by targeting those within the minority groups, e.g. minimal years of experience) is going to be highlighted in the BIT Landing page as per the Mockup in my GSoC20 proposal.

epicadk · 2021-05-03T03:59:20Z

app/utils/bitschema_utils.py

+
+@unique
+class YearsOfExperience(Enum):
+    UNDER_ONE = "Less than a year"


I think it's better to just store numbers (number of years of experience) in the database instead of storing strings.

@epicadk , the reason I did it like this (in word instead of number) is because IMO it makes more sense when an organization open a mentoring program opportunity, they can state the target candidate based on a range of years of experience (e.g. between 3 to 5 years) instead of a distinct year 1,2,3, or 4 yr etc. This is also to make it easier for later when we make the logic which allows user to filter mentoring program based on certain categories (e.g. years of experience required "between 3 - 5 years" instead of selecting a particular number like 1,2,3 or 4 yr etc). This way it also limits the options to 5 only. Hope this makes sense 😉 .

We can still make the same query with databases ( possibly more optimized as well if we use indexing) further every year we could just ++ the count in the database rather than having to migrate the strings.

@epicadk , what do you mean by

further every year we could just ++ the count in the database rather than having to migrate the strings.

Do you mean we need to make a scheduler to automatically add one year to the years of experience of each users who have this data instead of users manually updating their years of experience from their end? We could do that I suppose, but we're at disadvantage when the user is no longer in the industry but hasn't cancelled their membership/delete their account with us as that would be an inefficient use of overhead, plus we don't know how many of our users are in this category 🤔 .

Let's wait for @isabelcosta's opinion on this 😉 .

devkapilbansal

@mtreacy002 code coverage is descreasing significantly. I think new test cases are also needed with changes.

mtreacy002 · 2021-05-05T20:39:40Z

@mtreacy002 code coverage is descreasing significantly. I think new test cases are also needed with changes.

Ok, will add the test for the database model then 👍

mtreacy002 · 2021-05-06T09:59:03Z

Hi @devkapilbansal & @epicadk , does any you have a suggestion on how to increase the test coverage? I really appreciate any input 😉

epicadk · 2021-05-06T10:01:06Z

Hi @devkapilbansal & @epicadk , does any you have a suggestion on how to increase the test coverage? I really appreciate any input 😉

90 to 95% Coverage is fine as long as the tests cover all possible cases.

isabelcosta · 2021-05-07T18:50:17Z

app/database/models/personal_background.py

+    user_id: An integer for storing the user's id.
+    gender: A string for storing the user's gender.
+    age: A string for storing the user's age.
+    ethnicity: A string for storing the user's wthnicity.


Suggested change

ethnicity: A string for storing the user's wthnicity.

ethnicity: A string for storing the user's ethnicity.

Hi @mtreacy002 this is not resolved yet

isabelcosta · 2021-05-07T18:53:04Z

tests/personal_background/test_database_model.py

+        background = PersonalBackgroundModel.query.filter_by(
+            user_id=self.user2.id
+        ).first()
+        self.assertTrue(background is not None)


just as a fyi, you can do also assertIsNotNone() I think it's like this 🤔
not that you need to change, just wanted to let you know ;)

I forgot about that 🤣 . Thanks @isabelcosta for the reminder. I've fixed it now 😉 .

isabelcosta · 2021-05-16T11:39:28Z

app/database/models/personal_background.py

+
+        # default values
+        self.others = None
+        self.is_public = False


I think this should not be a boolean option.
I would say this could be an enum to represent different visibility levels. E.g.:

Not visible to anyone

Visible to BIT only

Visible to Organizations only

Visible to all users (probably no one will select this though 🤔 )

isabelcosta

I will approve this, as this is AS IS from BIT backend code, but lets try to after this merge, address the 2 review comments we discussed in the Open Session:

removing religious questions
making personal background visibility an enum instead of a boolean

mtreacy002 · 2021-05-17T02:29:06Z

I will approve this, as this is AS IS from BIT backend code, but lets try to after this merge, address the 2 review comments we discussed in the Open Session:

removing religious questions

making personal background visibility an enum instead of a boolean

Noted. Thank you @isabelcosta . Will open the issues and work on these points once this pr is approved 😉

mtreacy002 · 2021-06-25T02:53:37Z

@isabelcosta , @vj-codes and @epicadk , can you please advise if there's anything else need to be done here? Thanks 😉

isabelcosta · 2021-06-27T23:09:22Z

@mtreacy002 have you tested this and are you confident this is working ok? Since we are merging to BIT branch, I trust you on this. Happy to merge, just give me the green light 🤗

mtreacy002 · 2021-06-28T01:33:53Z

@mtreacy002 have you tested this and are you confident this is working ok? Since we are merging to BIT branch, I trust you on this. Happy to merge, just give me the green light 🤗

@isabelcosta , I'm pretty sure I've tested it locally though I haven't written the test report since I was hoping others could help with the testing part. But, I'll do it asap and will write the test summary soon 😉.

vj-codes · 2021-06-28T05:34:28Z

@mtreacy002 I can also test this, all I need to do is pull it locally and run unittests right?

mtreacy002 · 2021-06-28T06:04:29Z

@mtreacy002 I can also test this, all I need to do is pull it locally and run unittests right?

That's right, @vj-codes . Thank you, that'll be great, please 😉

mtreacy002 marked this pull request as draft May 3, 2021 01:48

mtreacy002 added BIT This labels issues that need to be completed to help BIT Category: Coding Changes to code base or refactored code that doesn't fix a bug. Type: Enhancement New feature or request. labels May 3, 2021

mtreacy002 force-pushed the add-background branch from 7360366 to 231b3be Compare May 3, 2021 01:50

Feat: Add BIT personal background to MS schema

781ffe4

Add new migration script reformat to black linting

mtreacy002 force-pushed the add-background branch from 231b3be to 781ffe4 Compare May 3, 2021 01:52

mtreacy002 marked this pull request as ready for review May 3, 2021 01:59

mtreacy002 requested review from isabelcosta and epicadk May 3, 2021 01:59

epicadk reviewed May 3, 2021

View reviewed changes

Add comments to specify BIT extension

bbcdb42

epicadk approved these changes May 4, 2021

View reviewed changes

Fix typo in LGBT acronym

7f63bee

devkapilbansal suggested changes May 5, 2021

View reviewed changes

mtreacy002 added 4 commits May 6, 2021 14:49

Add background test

fd33624

add more test

dc4cee2

add another test

235aecf

run black linting

394c664

isabelcosta reviewed May 7, 2021

View reviewed changes

mtreacy002 added 3 commits May 9, 2021 06:35

refactor to assertIsNotNone and add more tests

759d39e

run black linting

9a7791f

fix test background not exist

f8c0c12

mtreacy002 requested a review from isabelcosta May 8, 2021 22:09

mtreacy002 requested review from devkapilbansal and epicadk May 8, 2021 22:10

epicadk approved these changes May 16, 2021

View reviewed changes

isabelcosta reviewed May 16, 2021

View reviewed changes

isabelcosta approved these changes May 16, 2021

View reviewed changes

isabelcosta added the Status: Needs Testing Work has been reviewed and needs the code tested by the quality assurance team. label May 16, 2021

This was referenced May 17, 2021

Feat: remove Religion from the personal background attributes #1104

Open

Feat: Refactor is_public boolean to visibility enum on personal background #1105

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat: Add BIT personal background to MS schema #1092

Feat: Add BIT personal background to MS schema #1092

mtreacy002 commented May 3, 2021

codecov bot commented May 3, 2021 •

edited

Loading

mtreacy002 commented May 3, 2021

epicadk May 3, 2021

epicadk May 3, 2021

mtreacy002 May 4, 2021

epicadk May 3, 2021

mtreacy002 May 5, 2021

epicadk May 3, 2021

mtreacy002 May 5, 2021

epicadk May 3, 2021

mtreacy002 May 5, 2021 •

edited

Loading

epicadk May 7, 2021

mtreacy002 May 8, 2021

devkapilbansal left a comment

mtreacy002 commented May 5, 2021

mtreacy002 commented May 6, 2021

epicadk commented May 6, 2021

isabelcosta May 7, 2021

devkapilbansal May 18, 2021

isabelcosta May 7, 2021

mtreacy002 May 8, 2021

isabelcosta May 16, 2021

isabelcosta left a comment

mtreacy002 commented May 17, 2021

mtreacy002 commented Jun 25, 2021

isabelcosta commented Jun 27, 2021

mtreacy002 commented Jun 28, 2021

vj-codes commented Jun 28, 2021

mtreacy002 commented Jun 28, 2021

	ethnicity: A string for storing the user's wthnicity.
	ethnicity: A string for storing the user's ethnicity.

Feat: Add BIT personal background to MS schema #1092

Are you sure you want to change the base?

Feat: Add BIT personal background to MS schema #1092

Conversation

mtreacy002 commented May 3, 2021

Description

Type of Change:

How Has This Been Tested?

Checklist:

codecov bot commented May 3, 2021 • edited Loading

Codecov Report

mtreacy002 commented May 3, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtreacy002 May 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devkapilbansal left a comment

Choose a reason for hiding this comment

mtreacy002 commented May 5, 2021

mtreacy002 commented May 6, 2021

epicadk commented May 6, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isabelcosta left a comment

Choose a reason for hiding this comment

mtreacy002 commented May 17, 2021

mtreacy002 commented Jun 25, 2021

isabelcosta commented Jun 27, 2021

mtreacy002 commented Jun 28, 2021

vj-codes commented Jun 28, 2021

mtreacy002 commented Jun 28, 2021

codecov bot commented May 3, 2021 •

edited

Loading

mtreacy002 May 5, 2021 •

edited

Loading