feat: Create end point last entries #215 #221

PaulRC-ioet · 2020-10-30T23:20:41Z

An endpoint that returns the latest time-entries created belonging to different projects

Angeluz-07

Good job.

Angeluz-07 · 2020-11-05T05:38:31Z

time_tracker_api/time_entries/time_entries_model.py

@@ -470,14 +474,56 @@ def get_all(self, conditions: dict = None, **kwargs) -> list:
            max_count=limit,
        )

+    def get_last_projects_worked(


We may want this function to be called something like : get_latest_entries or get_latest. I think get_last_project may be a bit misleading.

Angeluz-07 · 2020-11-05T05:42:17Z

time_tracker_api/time_entries/time_entries_namespace.py

@@ -256,6 +256,18 @@ def post(self):
        return time_entries_dao.create(ns.payload), HTTPStatus.CREATED




Can we add

TODO: Once this endpoint is working as expected in prod, review and remove unnecessary filter args. As we are using same attributes_filter as the get_all endpoint and some of the args are unnecessary for this endpoint.

why exactly? should not we write proper tests instead of adding this?

@enriquezrene We put this TODO, in order to discuss in the next meeting. about the filters that we probably going to use in this endpoint

why exactly? should not we write proper tests instead of adding this?

@enriquezrene in this part of the code, Paul copied functionality of another endpoint to make it work. It works fine as it , It only seems to me that there may be extra logic that could be avoided. For that reason, I wanted to add a reminder to review that later.

We can enforce to remove that extra logic now or later, I am ok with both options.

Lets do it now please

Angeluz-07 · 2020-11-05T05:50:05Z

time_tracker_api/time_entries/time_entries_namespace.py

@@ -256,6 +256,18 @@ def post(self):
        return time_entries_dao.create(ns.payload), HTTPStatus.CREATED


+@ns.route('/latest')
+class TimeEntries(Resource):


Even tough seems working fine with this, please let's rename this class to LatestTimeEntries or Latest or LatestEntries

I am really sick of this file, it is so hard to understand what the heck is happening here. Please @PaulRC-ioet let's move all the classes to their own separate files. If we find a good reason to keep some in the same file please let me know, otherwise, please refactor this file to have many small files. I know you did not do this, but it is a good time to do some refactor tasks here.

and I agree with @Angeluz-07 on renaming this class. Classes like this

@ns.route('/latest') class TimeEntries(Resource):

should remain here, but repositories, daos and all the other things please move them into their own separate files.

I am according to you @enriquezrene , but this refactors tasks I think we have to create another ticket to do this refactor. In order to keep for now the order

Please not, if you want to create another ticket but as I mentioned in the meeting, each time we update the backend let's Refactor the code we find

ok, so I will do some refactor task here...

Please not, if you want to create another ticket but as I mentioned in the meeting, each time we update the backend let's Refactor the code we find

Good idea.

but repositories, daos and all the other things please move them into their own separate files.

Got it.

enriquezrene · 2020-11-05T22:27:59Z

time_tracker_api/time_entries/time_entries_model.py

@@ -470,14 +474,56 @@ def get_all(self, conditions: dict = None, **kwargs) -> list:
            max_count=limit,
        )

+    def get_last_projects_worked(
+        self, conditions: dict = None, **kwargs


can we move the class TimeEntriesCosmosDBDao to its own file. The current time_entries_model.py file is too big, we can no keep adding more code in it. Reading the code is a nightmare, adding changes will become harder as time passes and more features are added. Method name might be get_latest_entries_by_project

enriquezrene · 2020-11-05T22:29:56Z

time_tracker_api/time_entries/time_entries_model.py

+        for id_project in projects_ids:
+            conditions.update({"project_id": id_project})
+
+            limit = 2


why limit 2, don't you need only one record?

enriquezrene · 2020-11-05T22:31:17Z

time_tracker_api/time_entries/time_entries_model.py

+                max_count=limit,
+            )
+
+            if len(latest) >= 1:


this should be

if len(latest)>0: ...

enriquezrene · 2020-11-05T22:32:34Z

time_tracker_api/time_entries/time_entries_model.py

+                custom_sql_conditions=custom_query,
+                date_range=date_range,
+                max_count=limit,
+            )


are not you missing an order by clause somewhere?

It is already getting the values in descendent order by default so, no order clause is needed.

enriquezrene · 2020-11-05T22:33:52Z

time_tracker_api/time_entries/time_entries_namespace.py

@@ -256,6 +256,18 @@ def post(self):
        return time_entries_dao.create(ns.payload), HTTPStatus.CREATED




why exactly? should not we write proper tests instead of adding this?

enriquezrene · 2020-11-05T22:36:52Z

time_tracker_api/time_entries/time_entries_namespace.py

@@ -256,6 +256,18 @@ def post(self):
        return time_entries_dao.create(ns.payload), HTTPStatus.CREATED


+@ns.route('/latest')
+class TimeEntries(Resource):


I am really sick of this file, it is so hard to understand what the heck is happening here. Please @PaulRC-ioet let's move all the classes to their own separate files. If we find a good reason to keep some in the same file please let me know, otherwise, please refactor this file to have many small files. I know you did not do this, but it is a good time to do some refactor tasks here.

enriquezrene · 2020-11-05T22:40:07Z

time_tracker_api/time_entries/time_entries_namespace.py

@@ -256,6 +256,18 @@ def post(self):
        return time_entries_dao.create(ns.payload), HTTPStatus.CREATED


+@ns.route('/latest')
+class TimeEntries(Resource):


and I agree with @Angeluz-07 on renaming this class. Classes like this

@ns.route('/latest') class TimeEntries(Resource):

should remain here, but repositories, daos and all the other things please move them into their own separate files.

feat: Create end point last entries #215

efff1aa

PaulRC-ioet requested review from enriquezrene and Angeluz-07 October 30, 2020 23:20

Angeluz-07 suggested changes Nov 5, 2020

View reviewed changes

feat: Create end point last entries #215

91cd576

PaulRC-ioet force-pushed the 215_ReturnLastEntries branch from efff1aa to 91cd576 Compare November 5, 2020 22:28

enriquezrene suggested changes Nov 5, 2020

View reviewed changes

feat: #215 refactor code

46d3fd4

josepato87 force-pushed the 215_ReturnLastEntries branch from 9e4a460 to 46d3fd4 Compare November 12, 2020 17:44

josepato87 closed this Nov 12, 2020

		@@ -256,6 +256,18 @@ def post(self):
		return time_entries_dao.create(ns.payload), HTTPStatus.CREATED

feat: Create end point last entries #215 #221

feat: Create end point last entries #215 #221

Uh oh!

Conversation

PaulRC-ioet commented Oct 30, 2020

Uh oh!

Angeluz-07 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!

Angeluz-07 Nov 6, 2020 • 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!

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!

Uh oh!

Angeluz-07 Nov 6, 2020 •

edited

Loading