Oron exercise #206

orontamir · 2021-02-04T23:01:39Z

new feature exercise in calendar

PureDreamer

very nice work, it seems you thought about it well!

PureDreamer · 2021-02-04T23:50:16Z

app/routers/exercise.py

+async def exercise(
+        request: Request,
+        session=Depends(get_db),
+        new_user=Depends(get_placeholder_user)):


This shouldn't be with the placeholder, the placeholder is for testing.

Fix the issue

PureDreamer · 2021-02-04T23:50:47Z

app/routers/exercise.py

+    user = session.query(User).filter_by(id=1).first()
+    if not user:
+        # create default user
+        session.add(new_user)
+        session.commit()
+        user = session.query(User).filter_by(id=1).first()


this also should not be here.

Fix the issue

PureDreamer · 2021-02-04T23:51:29Z

app/routers/exercise.py

+    else:
+        day = 1
+    exercise_day = str(config.EXERCISE_FILE.format(num=day))
+    # print(exercise_day)


Why is it here? You should remove it.

PureDreamer · 2021-02-04T23:56:08Z

app/routers/profile.py

+    is_active_exercise = True
+
+    # Update database
+    user.is_active_exercise = is_active_exercise


Why not from the beginning right as it is
user.is_active_exercise = True?

Fix the issue

PureDreamer · 2021-02-04T23:59:05Z

app/routers/user_exercise.py

+    """
+      Checking if user exercise with user id is exist
+    """
+    return len(get_user_exercise(session=session, user_id=userid)) == 1


why equals to 1?
if it is something that is of value True why not

if get_user_exercise(session=session, user_id=userid): return True else: return False

or even easier
return get_user_exercise(session=session, user_id=userid)

Fix the issue

codecov-io · 2021-02-05T08:46:15Z

Codecov Report

Merging #206 (6ff8680) into develop (21faa13) will decrease coverage by 1.88%.
The diff coverage is 59.15%.

@@             Coverage Diff             @@
##           develop     #206      +/-   ##
===========================================
- Coverage    99.22%   97.34%   -1.89%     
===========================================
  Files           40       42       +2     
  Lines         1546     1617      +71     
===========================================
+ Hits          1534     1574      +40     
- Misses          12       43      +31

Impacted Files	Coverage Δ
app/main.py	`96.55% <ø> (ø)`
app/routers/profile.py	`89.32% <35.29%> (-10.68%)`	⬇️
app/routers/exercise.py	`50.00% <50.00%> (ø)`
app/routers/user_exercise.py	`73.07% <73.07%> (ø)`
app/database/models.py	`96.63% <100.00%> (+0.17%)`	⬆️
app/routers/telegram.py	`94.11% <0.00%> (-5.89%)`	⬇️
app/telegram/handlers.py	`98.38% <0.00%> (-1.62%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 21faa13...6ff8680. Read the comment docs.

ivarshav · 2021-02-05T13:01:22Z

app/main.py

@@ -46,11 +46,13 @@ def create_tables(engine, psql_environment):
    search.router,
    telegram.router,
    whatsapp.router,
+    exercise.router,


Please keep alphabetic order :)

fix the issue

ivarshav · 2021-02-05T13:02:07Z

app/routers/exercise.py

+@router.get("/")
+async def exercise(
+        request: Request,
+        session=Depends(get_db)


Plese add session type

fix the issue

ivarshav · 2021-02-05T13:03:21Z

app/routers/exercise.py

+        session=Depends(get_db)
+        ):
+    """
+    If is active exercise = True


Consider:
""" Show user exercise for a specific day if is_active_exercise is ture. """

fix the issue

ivarshav · 2021-02-05T13:05:20Z

app/routers/exercise.py

+    Show user exercise for specific day
+    """
+    user = session.query(User).filter_by(id=1).first()
+    if not user:


I remember I saw creating a default user in other places in code.
Maybe consider extract that to a util or internal function, so all can get or create default user?

ivarshav · 2021-02-05T13:06:22Z

app/routers/exercise.py

+    user_exercise = get_user_exercise(session, user_id=user.id)
+    if user_exercise:
+        # Get exercise day
+        date_time_now = datetime.now()


maybe use date.today()?

ivarshav · 2021-02-05T13:06:38Z

app/routers/exercise.py

+        # Get exercise day
+        date_time_now = datetime.now()
+        delta = date_time_now - user_exercise[0].start_date
+        # All exercise split to 30 days


fix the issue

ivarshav · 2021-02-05T13:07:32Z

app/routers/user_exercise.py

+
+def get_user_exercise(session: Session, **param) -> list:
+    """Returns user exercise filter by user id."""
+


Remove blank line in line 35

ivarshav · 2021-02-05T13:08:41Z

app/routers/user_exercise.py

+
+
+def get_user_exercise(session: Session, **param) -> list:
+    """Returns user exercise filter by user id."""


It's actully filter by param, not only user_id.

You can pass user_id exceptly if you want, something like:
def get_user_exercise(session: Session, user_id, **param)

fix the issue

ivarshav · 2021-02-05T13:09:38Z

app/routers/user_exercise.py

+    """Returns user exercise filter by user id."""
+
+    try:
+        exercise = list(session.query(UserExercise).filter_by(**param))


It's a list of exercises, so calling the variable exercises makes more sense (same for the function name)

fix the issue

ivarshav · 2021-02-05T13:28:55Z

app/routers/exercise.py

+            password='',
+            email=''
+        )
+    # Get user exercise


Where do you check is_active_exercise value?

by default is_active exercise = false
I check it in exercse tab

ivarshav · 2021-02-05T13:29:29Z

app/routers/profile.py

@@ -52,6 +54,36 @@ async def profile(
    })


+@router.get("/start_exercise")
+async def start_exercise(session=Depends(get_db)):
+    user = session.query(User).filter_by(id=1).first()


Add TODO for the user id

ivarshav · 2021-02-05T13:29:37Z

app/routers/profile.py

@@ -52,6 +54,36 @@ async def profile(
    })


+@router.get("/start_exercise")
+async def start_exercise(session=Depends(get_db)):


Please add types

fix the issue

ivarshav · 2021-02-05T13:30:19Z

app/routers/profile.py

+    session.commit()
+
+    # create user exercise
+    print("create user exercise")


remove print, you can add log if you want

ivarshav · 2021-02-05T13:30:58Z

app/routers/profile.py

+@router.get("/stop_exercise")
+async def stop_exercise(session=Depends(get_db)):
+    """
+    Stop exercise


"""Stop exercise - set is_active_exercise to False"""

ivarshav · 2021-02-05T13:31:43Z

app/routers/profile.py

@@ -52,6 +54,36 @@ async def profile(
    })


+@router.get("/start_exercise")


start_exercise and stop_exercise have a lot of common code, DRY :)

ivarshav · 2021-02-05T13:34:07Z

app/routers/user_exercise.py

+
+def does_user_exercise_exist(session: Session, userid: int) -> bool:
+    """
+      Checking if user exercise with user id is exist


Consider: Check if a user exercise for user id exists.

fix the issue

ivarshav · 2021-02-05T13:34:37Z

app/routers/user_exercise.py

+    return user_exercise
+
+
+def does_user_exercise_exist(session: Session, userid: int) -> bool:


fix the issue

ivarshav · 2021-02-05T13:35:54Z

tests/test_a_telegram_asyncio.py

@@ -6,7 +6,7 @@
 from .asyncio_fixture import today_date
 from .client_fixture import get_test_placeholder_user
 from app.telegram.handlers import MessageHandler, reply_unknown_user
-from app.telegram.keyboards import DATE_FORMAT
+# from app.telegram.keyboards import DATE_FORMAT


ivarshav · 2021-02-05T13:36:04Z

tests/test_a_telegram_asyncio.py

-From {(chosen_date + timedelta(days=-1)).strftime('%d/%m %H:%M')} \
-to {(chosen_date + timedelta(days=1)).strftime('%d/%m %H:%M')}: \
-Cool (somewhen in two days) event.\n'''
+    """


ivarshav · 2021-02-05T13:36:42Z

tests/test_a_telegram_asyncio.py

@@ -231,16 +231,17 @@ async def test_reply_unknown_user():


 class TestBotClient:
-
+    """


ivarshav · 2021-02-05T13:36:42Z

tests/test_user_exercise.py

+
+
+class TestUserExercise:
+


Remove blank line in line 7

ivarshav · 2021-02-05T13:37:09Z

tests/test_user_exercise.py

+class TestUserExercise:
+
+    def test_create_user_exercise(self, session):
+        _user = create_user(


Consider using user fixture.

ivarshav · 2021-02-05T13:37:23Z

tests/test_user_exercise.py

+
+class TestUserExercise:
+
+    def test_create_user_exercise(self, session):


Consider change to static method

ivarshav · 2021-02-05T13:38:59Z

tests/user_fixture.py

+def user_exercise(session: Session, user: User) -> UserExercise:
+    test_user_exercise = create_model(
+        session, UserExercise,
+        user_id=11,


Why 11 and not user.id?

just for testing

yammesicka

Great job! Please take into account the comments of @ivarshav .

Also, Where are the images from? Are there free to use? Should we give credit?

yammesicka · 2021-02-07T05:59:56Z

app/templates/exercise.html

+            <div class="col-5">
+                <!-- Exercise -->
+                {% if user.is_active_exercise %}
+                    <div style="font-family: 'Assistant', sans-serif;text-align: center;">


Use CSS instead

fix the issue

yammesicka · 2021-02-07T06:00:40Z

app/routers/user_exercise.py

+
+
+def update_user_exercise(user: User, session: Session) -> UserExercise:
+    user_exercise = session.query(UserExercise).filter_by(user_id=user.id)\


Prefer splitting using parenthesis and not \

fix the issue

yammesicka · 2021-02-07T06:01:12Z

app/routers/user_exercise.py

+from app.internal.utils import save
+
+
+def create_user_exercise(user: User, session: Session) -> UserExercise:


On some function you put the user first, and on others the session is first. Stick to one convention

fix the issue

…into oron_exercise

orontamir · 2021-02-07T13:00:24Z

Hi Yam

Thanks for your PR

All images are from the application: "Lose Weight in 30 days" its free to use

orontamir · 2021-02-07T13:03:34Z

Hi Yam

Thanks for your PR

All images are from the application: "Lose Weight in 30 days" its free to use

orontamir added 8 commits February 5, 2021 00:12

add exercise to calendar

a557ed9

Merge branch 'develop' into oron_exercise

b069b84

fix warning

ae0f2f2

fix missing new line

8907a4f

fix config file

4e14436

Update unit test

5866aa6

fix blank line

731c0a9

fix trailing whitespace

67deec3

PureDreamer reviewed Feb 5, 2021

View reviewed changes

orontamir added 2 commits February 5, 2021 10:42

fix PureDreamer issues

16e2e0f

fix unused import

4186ead

ivarshav reviewed Feb 5, 2021

View reviewed changes

tests/test_a_telegram_asyncio.py

@@ -231,16 +231,17 @@ async def test_reply_unknown_user():

class TestBotClient:

"""

Copy link

Contributor

ivarshav Feb 5, 2021

Choose a reason for hiding this comment

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

?

ivarshav reviewed Feb 5, 2021

View reviewed changes

tests/test_user_exercise.py

class TestUserExercise:

Copy link

Contributor

ivarshav Feb 5, 2021

Choose a reason for hiding this comment

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

Remove blank line in line 7

ivarshav reviewed Feb 5, 2021

View reviewed changes

orontamir added 2 commits February 5, 2021 15:57

fix ivarshav's issues

96f514d

fix unit test

cb9bc6f

yammesicka reviewed Feb 7, 2021

View reviewed changes

orontamir added 3 commits February 7, 2021 14:32

Merge branch 'develop' into oron_exercise

fe6a45f

fix Yam Comment

62e5134

Merge branch 'oron_exercise' of https://github.com/orontamir/calendar …

6ff8680

…into oron_exercise

orontamir closed this Feb 7, 2021


		def get_user_exercise(session: Session, **param) -> list:
		"""Returns user exercise filter by user id."""

		@@ -52,6 +54,36 @@ async def profile(
		})


		@router.get("/start_exercise")

		return user_exercise


		def does_user_exercise_exist(session: Session, userid: int) -> bool:

		@@ -231,16 +231,17 @@ async def test_reply_unknown_user():


		class TestBotClient:

		"""


		class TestUserExercise:

		def test_create_user_exercise(self, session):



		def update_user_exercise(user: User, session: Session) -> UserExercise:
		user_exercise = session.query(UserExercise).filter_by(user_id=user.id)\

		from app.internal.utils import save


		def create_user_exercise(user: User, session: Session) -> UserExercise:

Oron exercise #206

Oron exercise #206

Uh oh!

Conversation

orontamir commented Feb 4, 2021

Uh oh!

PureDreamer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PureDreamer Feb 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PureDreamer Feb 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Feb 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

PureDreamer Feb 4, 2021 •

edited

Loading

PureDreamer Feb 4, 2021 •

edited

Loading

codecov-io commented Feb 5, 2021 •

edited

Loading