Skip to content
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.

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

feat: combine response status filters #3359

Merged
merged 10 commits into from
Aug 2, 2023

Conversation

gabrielmbmb
Copy link
Member

Description

This PR updates the GET /api/v1/me/datasets/{dataset_id}/records and POST /api/v1/me/datasets/{dataset_id}/records/search and the SearchEngine.search method, so records can be filtered using more than one response status (/api/v1/...?response_status=submitted&response_status=discarded).

Closes #3259

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested

Locally and the unit tests for the endpoints and SearchEngine have been updated to cover the cases in which more than one value for the response status filter is provided.

Checklist

  • I added relevant documentation
  • follows the style guidelines of this project
  • I did a self-review of my code
  • I made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • I filled out the contributor form (see text above)
  • I have added relevant notes to the CHANGELOG.md file (See https://keepachangelog.com/)

@gabrielmbmb gabrielmbmb added type: enhancement Indicates new feature requests area: api Indicates that an issue or pull request is related to the Fast API server or REST endpoints labels Jul 7, 2023
@gabrielmbmb gabrielmbmb added this to the v1.13.0 milestone Jul 7, 2023
@frascuchon frascuchon removed this from the v1.13.0 milestone Jul 17, 2023
@codecov
Copy link

codecov bot commented Aug 2, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (c0f7577) 90.65% compared to head (3102ff7) 90.66%.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #3359   +/-   ##
========================================
  Coverage    90.65%   90.66%           
========================================
  Files          249      249           
  Lines        13350    13360   +10     
========================================
+ Hits         12103    12113   +10     
  Misses        1247     1247           
Files Changed Coverage Δ
src/argilla/server/apis/v1/handlers/datasets.py 100.00% <100.00%> (ø)
src/argilla/server/contexts/datasets.py 99.15% <100.00%> (+0.01%) ⬆️
src/argilla/server/search_engine.py 91.60% <100.00%> (+0.36%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@gabrielmbmb gabrielmbmb merged commit 265c801 into develop Aug 2, 2023
16 checks passed
@gabrielmbmb gabrielmbmb deleted the feature/combine-response-status-filters branch August 2, 2023 12:47
keithCuniah pushed a commit that referenced this pull request Aug 3, 2023
# Description

This PR updates the `GET /api/v1/me/datasets/{dataset_id}/records` and
`POST /api/v1/me/datasets/{dataset_id}/records/search` and the
`SearchEngine.search` method, so records can be filtered using more than
one response status
(`/api/v1/...?response_status=submitted&response_status=discarded`).

Closes #3259

**Type of change**

- [x] New feature (non-breaking change which adds functionality)

**How Has This Been Tested**

Locally and the unit tests for the endpoints and `SearchEngine` have
been updated to cover the cases in which more than one value for the
response status filter is provided.

**Checklist**

- [ ] I added relevant documentation
- [x] follows the style guidelines of this project
- [x] I did a self-review of my code
- [ ] I made corresponding changes to the documentation
- [x] My changes generate no new warnings
- [x] I have added tests that prove my fix is effective or that my
feature works
- [ ] I filled out [the contributor form](https://tally.so/r/n9XrxK)
(see text above)
- [x] I have added relevant notes to the CHANGELOG.md file (See
https://keepachangelog.com/)

---------

Co-authored-by: Paco Aranda <francis@argilla.io>
alvarobartt added a commit that referenced this pull request Aug 23, 2023
…d}/records` (#3613)

# Description

This PR adds the `response_status` to `GET
/api/v1/datasets/{dataset_id}/records` too, as previously it was just
included for `GET /api/v1/me/datasets/{dataset_id}/records` at
#3359, which was blocking
other developments related to the record listing.

Besides that, we've also unified `list_records_by_dataset_id` and
`list_records_by_dataset_id_and_user_id` into
`list_records_by_dataset_id` with the `user_id` arg being optional, so
that the response filter based on the `user_id` is just applied when
`user_id is not None`.

**Type of change**

- [X] New feature (non-breaking change which adds functionality)

**How Has This Been Tested**

(Please describe the tests that you ran to verify your changes. And
ideally, reference `tests`)

- [X] Add unit tests for `list_dataset_records` using
`response_statuses` via `response_status` alias for `GET
/api/v1/datasets/{dataset_id}/records`

**Checklist**

- [ ] I added relevant documentation
- [X] follows the style guidelines of this project
- [X] I did a self-review of my code
- [ ] I made corresponding changes to the documentation
- [X] My changes generate no new warnings
- [X] I have added tests that prove my fix is effective or that my
feature works
- [ ] I filled out [the contributor form](https://tally.so/r/n9XrxK)
(see text above)
- [x] I have added relevant notes to the CHANGELOG.md file (See
https://keepachangelog.com/)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: api Indicates that an issue or pull request is related to the Fast API server or REST endpoints type: enhancement Indicates new feature requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[API] Support combine multiple record response status filters
2 participants