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

[VIRTS-2798] Operation Link Results Endpoint #2239

Merged
merged 6 commits into from
Aug 26, 2021

Conversation

bleepbop
Copy link
Contributor

@bleepbop bleepbop commented Aug 26, 2021

Description

Created new endpoint GET /operations/{id}/links/{link_id}/result.
In the v1 api, there was an endpoint for fetching encoded link results (rest_svc.display_result()). We didn't have this in the api v2 design, but it's necessary as some abilities don't have parsers, preventing some results from being added to link objects as facts.

Type of change

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

How Has This Been Tested?

Tested using Postman, comparing output to v1 API. Also included Pytest unit tests for this endpoint; the rest of the operation pytests will be merged in a separate PR.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works

Copy link
Contributor

@uruwhy uruwhy left a comment

Choose a reason for hiding this comment

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

Let's add a pytest unit test as well to make sure the behavior works as expected - you can try mocking the read_result_file method for the file service to avoid having to read/write files on disk

@codecov
Copy link

codecov bot commented Aug 26, 2021

Codecov Report

Merging #2239 (b9638f0) into master (952bf78) will decrease coverage by 0.69%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2239      +/-   ##
==========================================
- Coverage   67.74%   67.04%   -0.70%     
==========================================
  Files          90       92       +2     
  Lines        6963     7286     +323     
==========================================
+ Hits         4717     4885     +168     
- Misses       2246     2401     +155     
Impacted Files Coverage Δ
app/api/v2/handlers/operation_api.py 61.76% <100.00%> (ø)
app/api/v2/managers/operation_api_manager.py 29.41% <100.00%> (ø)
app/service/app_svc.py 50.28% <0.00%> (+0.57%) ⬆️
app/objects/c_operation.py 71.42% <0.00%> (+1.68%) ⬆️
app/objects/c_planner.py 88.88% <0.00%> (+1.85%) ⬆️
app/api/v2/handlers/base_api.py 70.96% <0.00%> (+3.22%) ⬆️
app/api/v2/responses.py 86.11% <0.00%> (+16.66%) ⬆️
app/objects/c_source.py 86.84% <0.00%> (+17.10%) ⬆️
... and 1 more

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 952bf78...b9638f0. Read the comment docs.

@bleepbop
Copy link
Contributor Author

Let's add a pytest unit test as well to make sure the behavior works as expected - you can try mocking the read_result_file method for the file service to avoid having to read/write files on disk

Added fixtures from operations api pytests, along with tests for the new endpoint

@bleepbop bleepbop requested a review from uruwhy August 26, 2021 15:27
Copy link
Contributor

@uruwhy uruwhy 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!

Copy link
Contributor

@iguannalin iguannalin left a comment

Choose a reason for hiding this comment

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

Tested with UI--the endpoint is only called when link output is "True" and the returned result is decoded and displayed to the user. LGTM!

@bleepbop bleepbop merged commit de19a91 into master Aug 26, 2021
@bleepbop bleepbop deleted the bleepbop/VIRTS-2798/operations-api-fact-bugfix branch August 26, 2021 22:57
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.

3 participants