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

Bugfix/include distinct requests #211 #212

Merged
merged 5 commits into from
Mar 25, 2021

Conversation

MRichards99
Copy link
Collaborator

@MRichards99 MRichards99 commented Mar 22, 2021

This PR will close #211

Description

This fixes a bug found when making an e2e test pass on DataGateway regarding included attributes on a distinct filter (linked issue has more details about this). I added some unit tests for this work, to test that this functionality works as intended (test_prepare_distinct_fields()). I increased test coverage a bit too - test_valid_distinct_attribute_mapping(), test_invalid_distinct_attribute_mapping() and changes in test_session_handling.py.

There's some dependency updates, as safety is requesting they be updated for the CI to pass.

Testing Instructions

For the request shown in the issue, this is the correct response (assuming the API is pointing at preprod's ICAT):

[
    {
        "dataset": {},
        "id": 30
    }
]
  • Review code
  • Check GitHub Actions build
  • Review changes to test coverage
  • Test that included/related entity attributes can be used on a distinct filter and return the correct response

Agile Board Tracking

Connect to #211

- Also expands the unit test now those nested input values are possible to occur in the API
- This should give a nice increase in test coverage, hopefully codecov agrees!
- Unrelated to the issue tagged in the commit, but after my previous commit I noticed this would be an easy win for test coverage percentages so quickly modified the tests to get the increased test coverage
@MRichards99 MRichards99 force-pushed the bugfix/include-distinct-requests-#211 branch from 084f16c to 02778a2 Compare March 22, 2021 13:02
@codecov
Copy link

codecov bot commented Mar 22, 2021

Codecov Report

Merging #212 (02778a2) into master (04eafd5) will increase coverage by 0.60%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #212      +/-   ##
==========================================
+ Coverage   86.94%   87.55%   +0.60%     
==========================================
  Files          28       28              
  Lines        2229     2233       +4     
  Branches      193      194       +1     
==========================================
+ Hits         1938     1955      +17     
+ Misses        250      241       -9     
+ Partials       41       37       -4     
Impacted Files Coverage Δ
datagateway_api/common/icat/query.py 84.68% <100.00%> (+8.98%) ⬆️
...i/src/resources/non_entities/sessions_endpoints.py 100.00% <0.00%> (+16.00%) ⬆️

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 04eafd5...02778a2. Read the comment docs.

@VKTB VKTB self-assigned this Mar 23, 2021
@MRichards99 MRichards99 merged commit 671b33d into master Mar 25, 2021
@MRichards99 MRichards99 deleted the bugfix/include-distinct-requests-#211 branch March 25, 2021 09:34
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.

Fix bug with requests containing include filter and distinct not using included attribute
2 participants