Skip to content

feat: Create end point last entries #215 #221

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
Closed

feat: Create end point last entries #215 #221

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
efff1aa
feat: Create end point last entries #215
PaulRC-ioet Oct 30, 2020
91cd576
feat: Create end point last entries #215
PaulRC-ioet Oct 30, 2020
46d3fd4
feat: #215 refactor code
PaulRC-ioet Nov 6, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions tests/time_tracker_api/time_entries/time_entries_namespace_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,26 @@ def test_list_all_time_entries(
dao_get_all_mock.assert_called_once()


def test_list_last_time_entries(
client: FlaskClient, mocker: MockFixture, valid_header: dict
):
from time_tracker_api.time_entries.time_entries_namespace import (
time_entries_dao,
)

dao_get_all_mock = mocker.patch.object(
time_entries_dao, 'get_last_projects_worked', return_value=[]
)

response = client.get(
"/time-entries/latest", headers=valid_header, follow_redirects=True
)

assert HTTPStatus.OK == response.status_code
assert [] == json.loads(response.data)
dao_get_all_mock.assert_called_once()


def test_get_time_entry_should_succeed_with_valid_id(
client: FlaskClient, mocker: MockFixture, valid_header: dict
):
Expand Down
70 changes: 63 additions & 7 deletions time_tracker_api/time_entries/time_entries_model.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,8 @@ def find_all_entries(
conditions=conditions,
custom_sql_conditions=custom_sql_conditions,
custom_params=custom_params,
max_count=kwargs.get("max_count", None),
offset=kwargs.get("offset", 0),
)
return time_entries

Expand All @@ -173,6 +175,7 @@ def count(
conditions: dict = None,
custom_sql_conditions: List[str] = None,
date_range: dict = None,
**kwargs,
):
conditions = conditions if conditions else {}
custom_sql_conditions = (
Expand Down Expand Up @@ -424,7 +427,8 @@ def get_all(self, conditions: dict = None, **kwargs) -> list:
conditions.update({"owner_id": event_ctx.user_id})

custom_query = self.build_custom_query(
is_admin=event_ctx.is_admin, conditions=conditions,
is_admin=event_ctx.is_admin,
conditions=conditions,
)
date_range = self.handle_date_filter_args(args=conditions)
limit = conditions.get("limit", None)
Expand All @@ -437,14 +441,56 @@ def get_all(self, conditions: dict = None, **kwargs) -> list:
max_count=limit,
)

def get_last_projects_worked(
Copy link
Contributor

@Angeluz-07 Angeluz-07 Nov 5, 2020

Choose a reason for hiding this comment

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

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.

self, conditions: dict = None, **kwargs
Copy link
Contributor

@enriquezrene enriquezrene Nov 5, 2020

Choose a reason for hiding this comment

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

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

) -> list:
event_ctx = self.create_event_context("read-many")
conditions.update({"owner_id": event_ctx.user_id})
custom_query = self.build_custom_query(
is_admin=event_ctx.is_admin,
conditions=conditions,
)
date_range = self.handle_date_filter_args(args=conditions)

project_dao = projects_model.create_dao()
projects = project_dao.get_all()
projects_ids = [project.id for project in projects]

activity_dao = activities_model.create_dao()
activities = activity_dao.get_all(
visible_only=False,
)

result = []
for id_project in projects_ids:
conditions.update({"project_id": id_project})

limit = 2
Copy link
Contributor

@enriquezrene enriquezrene Nov 5, 2020

Choose a reason for hiding this comment

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

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

latest = self.repository.find_all_entries(
event_ctx,
conditions=conditions,
custom_sql_conditions=custom_query,
date_range=date_range,
max_count=limit,
)
Copy link
Contributor

@enriquezrene enriquezrene Nov 5, 2020

Choose a reason for hiding this comment

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

are not you missing an order by clause somewhere?

Copy link
Contributor

@Angeluz-07 Angeluz-07 Nov 6, 2020

Choose a reason for hiding this comment

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

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


if len(latest) >= 1:
Copy link
Contributor

@enriquezrene enriquezrene Nov 5, 2020

Choose a reason for hiding this comment

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

this should be

if len(latest)>0: ...

result.append(latest[0])

add_activity_name_to_time_entries(result, activities)
add_project_info_to_time_entries(result, projects)

return result

def get_all_paginated(self, conditions: dict = None, **kwargs) -> list:
get_all_conditions = dict(conditions)
get_all_conditions.pop("length")
get_all_conditions.pop("start")
event_ctx = self.create_event_context("read-many")
get_all_conditions.update({"owner_id": event_ctx.user_id})
custom_query = self.build_custom_query(
is_admin=event_ctx.is_admin, conditions=get_all_conditions,
is_admin=event_ctx.is_admin,
conditions=get_all_conditions,
)
date_range = self.handle_date_filter_args(args=get_all_conditions)
records_total = self.repository.count(
Expand All @@ -455,7 +501,8 @@ def get_all_paginated(self, conditions: dict = None, **kwargs) -> list:
)
conditions.update({"owner_id": event_ctx.user_id})
custom_query = self.build_custom_query(
is_admin=event_ctx.is_admin, conditions=conditions,
is_admin=event_ctx.is_admin,
conditions=conditions,
)
date_range = self.handle_date_filter_args(args=conditions)
length = conditions.get("length", None)
Expand Down Expand Up @@ -499,7 +546,11 @@ def update(self, id, data: dict, description=None):
time_entry = self.repository.find(id, event_ctx)
self.check_whether_current_user_owns_item(time_entry)

return self.repository.partial_update(id, data, event_ctx,)
return self.repository.partial_update(
id,
data,
event_ctx,
)

def stop(self, id):
event_ctx = self.create_event_context("update", "Stop time entry")
Expand All @@ -509,7 +560,9 @@ def stop(self, id):
self.check_time_entry_is_not_stopped(time_entry)

return self.repository.partial_update(
id, {'end_date': current_datetime_str()}, event_ctx,
id,
{'end_date': current_datetime_str()},
event_ctx,
)

def restart(self, id):
Expand All @@ -520,15 +573,18 @@ def restart(self, id):
self.check_time_entry_is_not_started(time_entry)

return self.repository.partial_update(
id, {'end_date': None}, event_ctx,
id,
{'end_date': None},
event_ctx,
)

def delete(self, id):
event_ctx = self.create_event_context("delete")
time_entry = self.repository.find(id, event_ctx)
self.check_whether_current_user_owns_item(time_entry)
self.repository.delete(
id, event_ctx,
id,
event_ctx,
)

def find_running(self):
Expand Down
12 changes: 12 additions & 0 deletions time_tracker_api/time_entries/time_entries_namespace.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,6 +256,18 @@ def post(self):
return time_entries_dao.create(ns.payload), HTTPStatus.CREATED


Copy link
Contributor

@Angeluz-07 Angeluz-07 Nov 5, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@enriquezrene enriquezrene Nov 5, 2020

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@PaulRC-ioet PaulRC-ioet Nov 5, 2020

Choose a reason for hiding this comment

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

@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

Copy link
Contributor

@Angeluz-07 Angeluz-07 Nov 6, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@enriquezrene enriquezrene Nov 6, 2020

Choose a reason for hiding this comment

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

Lets do it now please

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

@Angeluz-07 Angeluz-07 Nov 5, 2020

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@enriquezrene enriquezrene Nov 5, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@enriquezrene enriquezrene Nov 5, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@PaulRC-ioet PaulRC-ioet Nov 5, 2020

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@enriquezrene enriquezrene Nov 5, 2020

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@PaulRC-ioet PaulRC-ioet Nov 5, 2020

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@Angeluz-07 Angeluz-07 Nov 6, 2020

Choose a reason for hiding this comment

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

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.

@ns.doc('list_latest_time_entries')
@ns.expect(attributes_filter)
@ns.marshal_list_with(time_entry)
@ns.response(HTTPStatus.NOT_FOUND, 'Time entry not found')
def get(self):
"""List the latest time entries"""
conditions = attributes_filter.parse_args()
return time_entries_dao.get_last_projects_worked(conditions=conditions)


@ns.route('/<string:id>')
@ns.response(HTTPStatus.NOT_FOUND, 'This time entry does not exist')
@ns.response(HTTPStatus.UNPROCESSABLE_ENTITY, 'The id has an invalid format')
Expand Down