feat: busiest day of the week graph #243

nadav-pesach · 2021-02-08T16:13:57Z

No description provided.

…into graph

yammesicka

Great start! I've added few insights. Please take a look :)

yammesicka · 2021-02-09T07:03:25Z

app/templates/agenda.html

+    <div class="col-4">
+        <div class="card-body">
+            <canvas id="myChart" width="400" height="400"></canvas>
+            <script src="../static/graph.js"></script>


Prefer loading all the scripts in the end of the body tag

done!
and also changed to:
<script src="{{ url_for('static', path='/graph.js') }}"></script>

yammesicka · 2021-02-09T07:04:36Z

app/static/graph.js

@@ -0,0 +1,47 @@
+function makeGraph(events) {


Which graph? Please change the names in the code to be more indicative :)

yammesicka · 2021-02-09T07:05:06Z

app/routers/agenda.py

@@ -39,6 +40,18 @@ def agenda(
    """Route for the agenda page, using dates range or exact amount of days."""

    user_id = 1  # there is no user session yet, so I use user id- 1.
+
+    start_date_graph, end_date_graph = calc_dates_range_for_agenda(0, 0, 7)


Why 7? Try to avoid magic numbers :)

yammesicka · 2021-02-09T07:05:15Z

app/routers/agenda.py

+        db, user_id, start_date_graph, end_date_graph
+    )
+    events_for_graph = {
+        str(start_date_graph + timedelta(i)): 0 for i in range(8)}


well when it was 7 it actually returned 8 days
I changed it to return only a week

yammesicka · 2021-02-09T07:05:50Z

app/routers/agenda.py

+
+    start_date_graph, end_date_graph = calc_dates_range_for_agenda(0, 0, 7)
+
+    events_objects_for_graph = agenda_events.get_events_per_dates(


The name of the variables here can be a bit better :)

codecov-io · 2021-02-09T11:53:23Z

Codecov Report

Merging #243 (2e25250) into develop (b8f94b2) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff            @@
##           develop     #243   +/-   ##
========================================
  Coverage    98.16%   98.17%           
========================================
  Files           70       70           
  Lines         3000     3012   +12     
========================================
+ Hits          2945     2957   +12     
  Misses          55       55

Impacted Files	Coverage Δ
app/internal/agenda_events.py	`100.00% <100.00%> (ø)`
app/routers/agenda.py	`100.00% <100.00%> (ø)`

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 b8f94b2...2e25250. Read the comment docs.

…into graph

yammesicka

Awesome! I've added few of my insights. Please take a look :)

yammesicka · 2021-02-10T11:46:12Z

app/routers/agenda.py

@@ -39,6 +40,21 @@ def agenda(
    """Route for the agenda page, using dates range or exact amount of days."""

    user_id = 1  # there is no user session yet, so I use user id- 1.
+
+    get_a_week_int = 6


why the get prefix? Also, it looks like a constant, consider changing it to ALL_CAPITAL

yammesicka · 2021-02-10T11:46:32Z

app/routers/agenda.py

+
+    get_a_week_int = 6
+    start_date_graph, end_date_graph = calc_dates_range_for_agenda(
+        0, 0, get_a_week_int)


Consider changing to keyword arguments to make it more readable

yammesicka · 2021-02-10T11:47:08Z

app/routers/agenda.py

@@ -39,6 +40,21 @@ def agenda(
    """Route for the agenda page, using dates range or exact amount of days."""

    user_id = 1  # there is no user session yet, so I use user id- 1.
+
+    get_a_week_int = 6


Move all the logic to another function, preferably in a different file.

yammesicka · 2021-02-10T11:47:32Z

app/routers/agenda.py

+            timedelta(i)): 0 for i in range(get_a_week_int + 1)}
+
+    for event_obj in events_this_week:
+        events_for_graph[str(event_obj.start.date())] += 1


Prefer splitting this to two lines for the sake of readability

yammesicka · 2021-02-10T11:47:48Z

app/routers/agenda.py

+        db, user_id, start_date_graph, end_date_graph
+    )
+    events_for_graph = {
+        str(start_date_graph +


I'm sure we can make these three lines bit more readable :) Please fix the line splitting

app/static/graph.js

yammesicka · 2021-02-10T11:49:20Z

app/templates/agenda.html

+        <div class="card-body">
+            <canvas id="myChart" width="400" height="400"></canvas>
+            <div class="container" id="graph">
+                <script src="https://cdn.jsdelivr.net/npm/chart.js@2.8.0"></script>


Move to the end of the <body> block

yammesicka · 2021-02-18T11:24:46Z

app/internal/agenda_events.py

@@ -89,3 +90,22 @@ def get_events_in_time_frame(
    """Yields all user's events in a time frame."""
    events = get_all_user_events(db, user_id)
    yield from filter_dates(events, start_date, end_date)
+
+
+def make_dict_for_graph_data(db: Session, user_id: int) -> Dict:


Specify what types the Dict contains: Dict[key, value]

yammesicka · 2021-02-18T11:26:54Z

app/templates/agenda.html

@@ -60,4 +69,5 @@ <h1 id="dates">{{ start_date.strftime("%d/%m/%Y") }} - {{ end_date.strftime("%d/
    {% endfor %}
  </div>

+</div>


You sure you need to close this div here?

yammesicka · 2021-02-18T11:27:36Z

app/templates/base.html

@@ -75,6 +75,8 @@
    <!-- This bootstrap version is needed here because of the toggler bug in the beta version-->
    <script src="https://stackpath.bootstrapcdn.com/bootstrap/5.0.0-alpha2/js/bootstrap.bundle.min.js" integrity="sha384-BOsAfwzjNJHrJ8cZidOg56tcQWfp6y72vEJ8xQ9w6Quywb24iOsW913URv1IS4GD" crossorigin="anonymous"></script>
    <script src="{{ url_for('static', path='/horoscope.js') }}"></script>
+    <script src="{{ url_for('static', path='/graph.js') }}"></script>
+    <script src="https://cdn.jsdelivr.net/npm/chart.js@2.8.0"></script>


We already have Chart.js 2.9.4 included, see few lines above

…into graph

yammesicka · 2021-02-20T11:52:51Z

app/internal/agenda_events.py

+    """create a dict with number of events per day for current week"""
+    WEEK_DAYS = 6
+    start_date, end_date = agenda.calc_dates_range_for_agenda(
+        0, 0, WEEK_DAYS)


add parameter names to make the call more understandable: X=0, Y=0, ..

I actually changed it to make it more readable

yammesicka · 2021-02-20T12:00:34Z

app/internal/agenda_events.py

+    WEEK_DAYS = 6
+    start_date, end_date = agenda.calc_dates_range_for_agenda(
+        0, 0, WEEK_DAYS)
+
+    events_this_week = get_events_per_dates(
+        db, user_id, start_date, end_date
+    )
+    events_for_graph = {
+        str(start_date + timedelta(i)): 0 for i in range(WEEK_DAYS + 1)
+    }


This should be on a separate function

yammesicka · 2021-02-20T12:01:45Z

Probably the last set of fixes :)

… graph

app/internal/agenda_events.py

yammesicka · 2021-02-22T00:43:24Z

Resolve conflicts and send it to me for merge

nadav-pesach added 4 commits February 8, 2021 17:17

feat: busiest day of the week graph

91a73aa

Merge branch 'develop' of https://github.com/PythonFreeCourse/calendar …

cf4e6b3

…into graph

git add

16ece2a

test fix

fe478cf

yammesicka reviewed Feb 9, 2021

View reviewed changes

git issus

7897f91

nadav-pesach added 8 commits February 9, 2021 13:58

fix issues

0a5f18a

fix datetime in event

0194168

Merge branch 'develop' of https://github.com/PythonFreeCourse/calendar …

cd38c1a

…into graph

gix

e5c5db0

fix

e01dac8

fix...

10df159

fix?

d3803bd

fix dt

ee2d9e7

nadav-pesach requested a review from yammesicka February 9, 2021 15:20

yammesicka reviewed Feb 10, 2021

View reviewed changes

nadav-pesach added 3 commits February 17, 2021 11:07

fix issues

25a0caf

fix all issues

7ad3f20

fix graph

4fcf042

nadav-pesach requested a review from yammesicka February 17, 2021 10:20

yammesicka requested changes Feb 18, 2021

View reviewed changes

nadav-pesach added 2 commits February 18, 2021 14:02

small fix

d512159

Merge branch 'develop' of https://github.com/PythonFreeCourse/calendar …

ecad9a0

…into graph

nadav-pesach requested a review from yammesicka February 18, 2021 12:09

Merge branch 'develop' into graph

2e25250

yammesicka requested changes Feb 20, 2021

View reviewed changes

nadav-pesach added 2 commits February 20, 2021 15:28

small fix

fa528d0

Merge branch 'graph' of https://github.com/nadav-pesach/calendar into…

496e8ca

… graph

nadav-pesach added 2 commits February 20, 2021 15:38

small fix

4662dab

fix name

5906a2a

nadav-pesach requested a review from yammesicka February 20, 2021 13:46

ivarshav reviewed Feb 21, 2021

View reviewed changes

app/internal/agenda_events.py Show resolved Hide resolved

yammesicka approved these changes Feb 22, 2021

View reviewed changes

Resolved conflicts

0f1d94a

nadav-pesach requested a review from yammesicka February 22, 2021 08:51

yammesicka merged commit 9c9c499 into PythonFreeCourse:develop Feb 22, 2021


		start_date_graph, end_date_graph = calc_dates_range_for_agenda(0, 0, 7)

		events_objects_for_graph = agenda_events.get_events_per_dates(

feat: busiest day of the week graph #243

feat: busiest day of the week graph #243

Uh oh!

Conversation

nadav-pesach commented Feb 8, 2021

Uh oh!

yammesicka 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!

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!

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

Uh oh!

Codecov Report

Uh oh!

yammesicka 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!

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!

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

codecov-io commented Feb 9, 2021 •

edited

Loading