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

Empty Where Filter Input on Count Endpoints #326

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

VKTB
Copy link
Contributor

@VKTB VKTB commented Feb 8, 2022

This PR will close #309

Description

The changes in this PR make the count endpoints work when they receive an empty where filter input (where={}). Also added some extra code so that FilterError is raised when the where filter input is not an object.

Testing Instructions

  • Review code
  • Check GitHub Actions build
  • If icatdb Generator Script Consistency Test CI job fails, is this because of a deliberate change made to the script to change generated data (which isn't actually a problem) or is here an underlying issue with the changes made?
  • Review changes to test coverage
  • Does this change mean a new patch, minor or major version should be made? If so, does one of the commit messages feature fix:, feat: or BREAKING CHANGE: so a release is automatically made via GitHub Actions upon merge?

@VKTB
Copy link
Contributor Author

VKTB commented Feb 8, 2022

The failing test jobs will be fixed by #325

@VKTB VKTB marked this pull request as ready for review February 8, 2022 15:37
@VKTB VKTB requested a review from MRichards99 February 8, 2022 15:37
Copy link
Collaborator

@MRichards99 MRichards99 left a comment

Choose a reason for hiding this comment

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

Looks good, just need to reword the error message as per my comment and merge master in once #325 is merged so the CI passes.

@codecov
Copy link

codecov bot commented Feb 10, 2022

Codecov Report

Merging #326 (e23f0c4) into master (80222ee) will increase coverage by 0.00%.
The diff coverage is 93.54%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #326   +/-   ##
=======================================
  Coverage   92.62%   92.62%           
=======================================
  Files          43       43           
  Lines        3199     3242   +43     
  Branches      318      325    +7     
=======================================
+ Hits         2963     3003   +40     
- Misses        198      200    +2     
- Partials       38       39    +1     
Impacted Files Coverage Δ
datagateway_api/src/search_api/helpers.py 97.53% <ø> (-2.47%) ⬇️
...gateway_api/src/search_api/query_filter_factory.py 95.65% <93.54%> (+0.09%) ⬆️
datagateway_api/src/search_api/filters.py 89.15% <0.00%> (-0.46%) ⬇️
...agateway_api/src/resources/search_api_endpoints.py 100.00% <0.00%> (ø)
datagateway_api/src/search_api/models.py 96.69% <0.00%> (+0.16%) ⬆️
datagateway_api/src/api_start_utils.py 86.29% <0.00%> (+0.45%) ⬆️

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 80222ee...e23f0c4. Read the comment docs.

@VKTB VKTB requested a review from MRichards99 February 10, 2022 10:40
MRichards99
MRichards99 previously approved these changes Feb 10, 2022
Copy link
Collaborator

@MRichards99 MRichards99 left a comment

Choose a reason for hiding this comment

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

Looks good, we should probably wait to merge this PR so we can keep one in reserve for the DOI minting discussed on Slack.

@MRichards99
Copy link
Collaborator

This was waiting for a DOI to be minted for the search API. I believe this is unlikely to happen now, so the merge conflicts should be resolved and the PR should be merged.

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.

Empty Dict on Query Param for Count Endpoints
2 participants