Dev: Add BIT user attributes to MS User #1074

mtreacy002 · 2021-04-18T03:52:45Z

Description

Adding BIT user attributes to MS User as the first step for full BIT-MS integration

Fixes #1073

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?

The local_data.db has successfully added BIT User specific attributes after running migration script.

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

epicadk · 2021-04-18T04:03:14Z

app/utils/bitschema_utils.py

+
+
+@unique
+class Timezone(Enum):


Is this required? There should be something inbuilt to do the same thing. I know there is in java.

I'm open to refactoring this Timezone enum to a more efficient way if someone can pointed out an inbuilt package that is available out there. We can try to refactor the Timezone in the BIT repo first instead of here to see if it works coz BIT schema already the tests written for the endpoints that are using this Timezone enum.

https://www.iana.org/time-zones

Instead of this enum we could do this it's an inbuilt package. But implementing this means that the tz package would have to be the same for all the application.

@epicadk , unfortunately, this zoneinfo module only available for Python >= 3.9.

MS backend is still using Python 3.6 for gh-action, so the gh action build/test will fail unless we change this.

Also, this enum is currently used in BIT frontend in timezones.js as a constant so that we can provide the dropdown options to user to prevent them inserting timezone that doesn't fall within the accepted value of backend timezone enum (which is according to the pytz list). Do you have any suggestions how we do this if we refactor backend to zoneinfo? What should we use as constant at the frontend?

We'll run a matrix build. I'll create an issue. Mentorship system runs fine on python version 3.9.

@mtreacy002 can you now use python 3.9 and use zoneinfo?

epicadk · 2021-04-18T04:05:17Z

app/database/models/user.py

        }

    def __repr__(self):
-        """Returns the user's name and username. """
-        return f"User name {self.name} . Username is {self.username} ."
+        """Returns the user's name, username, whether or not they are an organization representative and their timezone. """


Requires approval from @isabelcosta

Yep..., I actually think it won't be too much of a problem if we don't include the is_organization_rep or Timezone in here since they can be abstracted later once we grab the user data using GET /user api call 🤔. I will just remove them for now 😁.

mtreacy002 · 2021-04-18T05:33:10Z

My approached in this PR was from an angle of a new contributor who has succesfully completed the local setup.
The followings are the steps done on this PR:

updated local develop branch by pulling the latest commit from upstream.
created a new pr branch
run python run.py comand => this created the local_data.db. Notice that the alembic version folder din't exist in the db.

Registered a new user and confirm email (this is now what most contributors would have on their db where they have data but no alembic version folder as contributors have never run migration script before.
run initial migration using command "flask db migrate -m "Initial migration" => this added alembic version folder into the local_data.db.

Added the render_as_batch condition in run.py so that running flask-db-upgrade won't return this error

Made the schema changes in UserModel by adding BIT user specific attribute. Then run another migration command flask db migrate -m "<message>" where <message> is a clear message what the change was about. This was used as the alembic version title. This round running the script actually created the new alembic script version as it should.

Fixed migration script so running flask db upgrade won't return this error

The culprit was shown below

And here's the fix

Refreshed the local_data.db on the sqlite db viewer tool and saw the new attributes were added (screenshot given in pr descripction above)

epicadk · 2021-04-18T06:01:16Z

A tip for @isabelcosta and other reviewers you can the changes @mtreacy002 has made here

mtreacy002 · 2021-04-18T06:15:01Z

@epicadk, there were soooo many other commits there included in this PR 😱. Must be because the bit branch hasn't been synced to the develop branch from the admin/maintainers side. Is it possible for you to do this? I think once bit branch is updated, the irrelevant commits will disappear from the pr, right?

epicadk · 2021-04-18T06:16:54Z

I think they will yes. Also can you please add BIT to github actions?

mtreacy002 · 2021-04-18T06:50:24Z

I think they will yes. Also can you please add BIT to github actions?

Done

mtreacy002 · 2021-04-18T06:57:40Z

@epicadk, should I also add bit to gh-action docs.yml, or wait till we have docs related issue to add bit to the docs workflow?

codecov · 2021-04-18T07:01:03Z

Codecov Report

Merging #1074 (e2c16ae) into bit (ca4728a) will increase coverage by 1.48%.
The diff coverage is 99.84%.

❗ Current head e2c16ae differs from pull request most recent head 81f739c. Consider uploading reports for the commit 81f739c to get more accurate results

@@            Coverage Diff             @@
##              bit    #1074      +/-   ##
==========================================
+ Coverage   92.90%   94.39%   +1.48%     
==========================================
  Files          38       39       +1     
  Lines        2073     2623     +550     
==========================================
+ Hits         1926     2476     +550     
  Misses        147      147

Impacted Files	Coverage Δ
app/api/models/mentorship_relation.py	`100.00% <ø> (ø)`
app/api/models/task.py	`100.00% <ø> (ø)`
app/utils/bitschema_utils.py	`99.83% <99.83%> (ø)`
app/api/dao/mentorship_relation.py	`96.11% <100.00%> (ø)`
app/api/dao/task.py	`98.48% <100.00%> (ø)`
app/api/dao/task_comment.py	`100.00% <100.00%> (ø)`
app/api/dao/user.py	`85.77% <100.00%> (-0.06%)`	⬇️
app/api/resources/admin.py	`88.13% <100.00%> (-0.58%)`	⬇️
app/api/resources/task.py	`100.00% <100.00%> (ø)`
app/api/resources/user.py	`89.75% <100.00%> (-0.66%)`	⬇️
... and 22 more

mtreacy002 · 2021-04-18T07:07:05Z

Codecov Report

❗ No coverage uploaded for pull request base (bit@479f1c7). Click here to learn what that means.
The diff coverage is 99.84%.
@@          Coverage Diff           @@
##             bit    #1074   +/-   ##
======================================
  Coverage       ?   94.39%           
======================================
  Files          ?       39           
  Lines          ?     2623           
  Branches       ?        0           
======================================
  Hits           ?     2476           
  Misses         ?      147           
  Partials       ?        0           
Impacted Files Coverage Δ
app/api/models/mentorship_relation.py 100.00% <ø> (ø)
app/api/models/task.py 100.00% <ø> (ø)
app/utils/bitschema_utils.py 99.83% <99.83%> (ø)
app/api/dao/mentorship_relation.py 96.11% <100.00%> (ø)
app/api/dao/task.py 98.48% <100.00%> (ø)
app/api/dao/task_comment.py 100.00% <100.00%> (ø)
app/api/dao/user.py 85.77% <100.00%> (ø)
app/api/resources/admin.py 88.13% <100.00%> (ø)
app/api/resources/task.py 100.00% <100.00%> (ø)
app/api/resources/user.py 89.75% <100.00%> (ø)
... and 6 more

@epicadk , can you suggest what went wrong here? 🤔
Is it just bcoz this is the first report so nothing to compare? That should be fine then, right?

epicadk · 2021-04-18T08:02:54Z

@epicadk, should I also add bit to gh-action docs.yml, or wait till we have docs related issue to add bit to the docs workflow?

Yup I think you can wait for this.

epicadk · 2021-04-18T08:04:10Z

@epicadk , can you suggest what went wrong here? 🤔
Is it just bcoz this is the first report so nothing to compare? That should be fine then, right?

Yup that's right. It's fine.

mtreacy002 · 2021-04-18T11:19:26Z

@isabelcosta , I have resolved the conflict, can you please review again 😉

isabelcosta · 2021-04-18T12:26:25Z

auch @mtreacy002 can you squash all these commits into one? I see an issue here, and I bet it's because of the way that I did the sync 🙈

epicadk · 2021-04-18T12:31:35Z

auch @mtreacy002 can you squash all these commits into one? I see an issue here, and I bet it's because of the way that I did the sync 🙈

Nope rebasing won't be possible. It would still show the error.

mtreacy002 · 2021-04-19T20:17:55Z

@isabelcosta , do you still face the conflict on merging? Do you want me to try squash merge all the commits from my end?

epicadk · 2021-04-20T03:36:46Z

@isabelcosta , do you still face the conflict on merging? Do you want me to try squash merge all the commits from my end?

You could try but I'm guessing it would still show merge conflicts after squashing. What you could do is. Rebase them into a single commit and then merge the upstream branch.

mtreacy002 · 2021-04-22T01:00:20Z

Done, @isabelcosta and @epicadk , I've squashed and force-pushed the commits. Can you please check again if this still causing conflicts at your end when merging?

epicadk · 2021-04-22T04:29:49Z

Done, @isabelcosta and @epicadk , I've squashed and force-pushed the commits. Can you please check again if this still causing conflicts at your end when merging?

Yeps this might work.

isabelcosta · 2021-04-25T10:42:04Z

run.py

@@ -14,7 +14,7 @@ def create_app(config_filename: str) -> Flask:

    db.init_app(app)

-    migrate = Migrate(app, db)
+    migrate = Migrate(app, db, render_as_batch=True)


what does this do? 🤓

isabelcosta · 2021-04-25T10:42:41Z

migrations/versions/1272e4b54a32_add_user_bitschema_attribute.py

+
+
+def downgrade():
+    # ### commands auto generated by Alembic - please adjust! ###


These comments from alembic can be removed right?

@isabelcosta , done 😉

epicadk · 2021-04-25T11:20:42Z

Added the render_as_batch condition in run.py so that running flask-db-upgrade won't return this error

@isabelcosta

mtreacy002 · 2021-04-26T01:22:58Z

@vj-codes , I remember you asked last time whether we need to add Black to requirements.txt and my answered to that was that it's not needed. However, now that I encountered this issue with local Black version passing files but Black version in gh-action failing it, it seems fair to put Black to the `requirements.txt so that we can make sure all contributors running the same version locally that we'll match to the latest stable version on gh-action. What do you think of this? cc @epicadk

epicadk · 2021-04-29T15:45:17Z

@vj-codes , I remember you asked last time whether we need to add Black to requirements.txt and my answered to that was that it's not needed. However, now that I encountered this issue with local Black version passing files but Black version in gh-action failing it, it seems fair to put Black to the `requirements.txt so that we can make sure all contributors running the same version locally that we'll match to the latest stable version on gh-action. What do you think of this? cc @epicadk

Yup seems like a nice idea. But maybe add it to dev-requirements.txt? Also sorry for the delay I have some other commitments so I can't really attend the ms open session. I'll try to attend the upcoming one so we can discuss and possibly merge this. I actually think we can merge this and handle the timezone case later.

epicadk · 2021-05-01T05:52:25Z

@isabelcosta no other changes from my side, can this be merged?

isabelcosta · 2021-05-01T14:15:29Z

@mtreacy002 can you try to solve the merge conflicts please? Me, @vj-codes and @epicadk did some merges and branch recreation, and now that we added the matrix python version thing to GH Actions, there are conflicts.

remove BIT attrributes from being returned in _repr_(self) remove unnecessary brackets Fix description add bit branch to github action fix black linting error Remove alembic auto-generated comments

mtreacy002 · 2021-05-02T00:52:21Z

I've solved the conflict @isabelcosta 😉

isabelcosta

Will approve and merge once it has 2 approvals.

Points raised:

We discussed that, timezones will be refactored later, to come from zoneinfo module.
Instructions on how to run migrations should go to the README.

epicadk

👍🏼

mtreacy002 requested review from isabelcosta and epicadk April 18, 2021 03:53

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 Apr 18, 2021

epicadk reviewed Apr 18, 2021

View reviewed changes

mtreacy002 force-pushed the add-bitschema-user branch from b726aa3 to c5dd57c Compare April 22, 2021 00:36

isabelcosta reviewed Apr 25, 2021

View reviewed changes

vj-codes mentioned this pull request Apr 26, 2021

chore:improved github actions to use a matrix #1079

Merged

4 tasks

isabelcosta closed this May 1, 2021

isabelcosta deleted the branch anitab-org:bit May 1, 2021 14:12

isabelcosta reopened this May 1, 2021

Dev: Add BIT user attributes to MS User table

81f739c

remove BIT attrributes from being returned in _repr_(self) remove unnecessary brackets Fix description add bit branch to github action fix black linting error Remove alembic auto-generated comments

mtreacy002 force-pushed the add-bitschema-user branch from e2c16ae to 81f739c Compare May 2, 2021 00:43

isabelcosta approved these changes May 2, 2021

View reviewed changes

epicadk approved these changes May 2, 2021

View reviewed changes

isabelcosta merged commit f25a0b8 into anitab-org:bit May 2, 2021



		def downgrade():
		# ### commands auto generated by Alembic - please adjust! ###



		@unique
		class Timezone(Enum):

Dev: Add BIT user attributes to MS User #1074

Dev: Add BIT user attributes to MS User #1074

Conversation

mtreacy002 commented Apr 18, 2021

Description

Type of Change:

How Has This Been Tested?

Checklist:

epicadk Apr 18, 2021

Choose a reason for hiding this comment

mtreacy002 Apr 18, 2021

Choose a reason for hiding this comment

epicadk Apr 18, 2021

Choose a reason for hiding this comment

epicadk Apr 18, 2021 • edited Loading

Choose a reason for hiding this comment

mtreacy002 Apr 22, 2021 • edited Loading

Choose a reason for hiding this comment

epicadk Apr 22, 2021 • edited Loading

Choose a reason for hiding this comment

isabelcosta May 1, 2021 • edited Loading

Choose a reason for hiding this comment

epicadk Apr 18, 2021

Choose a reason for hiding this comment

mtreacy002 Apr 18, 2021 • edited Loading

Choose a reason for hiding this comment

mtreacy002 commented Apr 18, 2021 • edited Loading

epicadk commented Apr 18, 2021

mtreacy002 commented Apr 18, 2021 • edited Loading

epicadk commented Apr 18, 2021 • edited Loading

mtreacy002 commented Apr 18, 2021

mtreacy002 commented Apr 18, 2021

codecov bot commented Apr 18, 2021 • edited Loading

Codecov Report

mtreacy002 commented Apr 18, 2021 • edited Loading

Codecov Report

epicadk commented Apr 18, 2021

epicadk commented Apr 18, 2021

mtreacy002 commented Apr 18, 2021

isabelcosta commented Apr 18, 2021

epicadk commented Apr 18, 2021 • edited Loading

mtreacy002 commented Apr 19, 2021 • edited Loading

epicadk commented Apr 20, 2021 • edited Loading

mtreacy002 commented Apr 22, 2021

epicadk commented Apr 22, 2021

isabelcosta Apr 25, 2021

Choose a reason for hiding this comment

isabelcosta Apr 25, 2021

Choose a reason for hiding this comment

mtreacy002 Apr 26, 2021

Choose a reason for hiding this comment

epicadk commented Apr 25, 2021

mtreacy002 commented Apr 26, 2021

epicadk commented Apr 29, 2021 • edited Loading

epicadk commented May 1, 2021

isabelcosta commented May 1, 2021

mtreacy002 commented May 2, 2021

isabelcosta left a comment

Choose a reason for hiding this comment

epicadk left a comment

Choose a reason for hiding this comment

epicadk Apr 18, 2021 •

edited

Loading

mtreacy002 Apr 22, 2021 •

edited

Loading

epicadk Apr 22, 2021 •

edited

Loading

isabelcosta May 1, 2021 •

edited

Loading

mtreacy002 Apr 18, 2021 •

edited

Loading

mtreacy002 commented Apr 18, 2021 •

edited

Loading

mtreacy002 commented Apr 18, 2021 •

edited

Loading

epicadk commented Apr 18, 2021 •

edited

Loading

codecov bot commented Apr 18, 2021 •

edited

Loading

mtreacy002 commented Apr 18, 2021 •

edited

Loading

epicadk commented Apr 18, 2021 •

edited

Loading

mtreacy002 commented Apr 19, 2021 •

edited

Loading

epicadk commented Apr 20, 2021 •

edited

Loading

epicadk commented Apr 29, 2021 •

edited

Loading