Skip to content

Conversation

@enriquezrene
Copy link
Contributor

fix: #91 adding filter by attributes

Copy link
Contributor

@EliuX EliuX left a comment

Choose a reason for hiding this comment

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

This implementation is not compliant with the format requested in the ticket for grouping the filter attributes.

def get(self):
"""List all customers"""
return customer_dao.get_all()
return customer_dao.get_all(conditions=request.args)
Copy link
Contributor

@EliuX EliuX Apr 21, 2020

Choose a reason for hiding this comment

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

What if you have some query like this?

/activities?a=1&b=2&pageSize=0&orders["start_date"]="DESC"&limit=1

This is not going to work, because a and b are the attributes but you are filtering for whatever it is specified in the request arguments.

Copy link
Contributor

Choose a reason for hiding this comment

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

In addition, it would be nice to filter per namespace that the attributes you want to filter by are valid.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What if you have some query like this?

/activities?a=1&b=2&pageSize=0&orders["start_date"]="DESC"&limit=1

This is not going to work, because a and b are the attributes but you are filtering for whatever it is specified in the request arguments.

Do we have filtering and sorting arguments right now?

Copy link
Contributor

Choose a reason for hiding this comment

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

The repository has sorting capabilities right now. For instance, The repository for time entries orders the results by start_date. We could do this dynamically in the future.

@EliuX
Copy link
Contributor

EliuX commented Apr 21, 2020

Also, please rename the commit message.
Instead of fix, which is a patch, use feat, because you are adding a new feature.
For Github, you can use Fix or Close and the number of the issue. A final message for the commit could be:

feat: Close #91 Add filter by attributes to get all

This will increase the version with a minor update.

@enriquezrene enriquezrene force-pushed the 93-filter-projects-and-project_types-by-customer branch from 6f0a906 to beb8c84 Compare April 23, 2020 20:01
EliuX
EliuX previously approved these changes Apr 24, 2020
@EliuX EliuX force-pushed the 93-filter-projects-and-project_types-by-customer branch from a4b6cb8 to 7f8b00d Compare April 24, 2020 01:13
@EliuX EliuX dismissed their stale review April 24, 2020 01:13

I should not approve my own changes

@EliuX EliuX force-pushed the 93-filter-projects-and-project_types-by-customer branch from 7f8b00d to 44a4260 Compare April 24, 2020 15:35
@EliuX EliuX merged commit e7cd807 into master Apr 24, 2020
@EliuX EliuX deleted the 93-filter-projects-and-project_types-by-customer branch April 24, 2020 15:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Find all items filtering by attributes

4 participants