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

Show admin upload on search by upload id #2757

Merged
merged 13 commits into from Oct 4, 2019

Conversation

@kahlouie
Copy link
Contributor

commented Oct 1, 2019

Description

When an upload has a virus, we need admins to be able to get the information about the upload, including the Move Locator.

Reviewer Notes

I had a question around the associations that I used to get move locator from upload. Any advice on this would be lovely.

Setup

Add any steps or code to run in this section to help others prepare to run your code:

make db_dev_e2e_populate
make server_run
make admin_client_run

go to adminlocal:3000. Click on the Search Upload by ID. Search for an upload (you can use 12b9e74c-f963-4810-866a-1183e02911b8 if you're using the e2e data). You should see all the required fields around the upload and the move locator.

Code Review Verification Steps

  • Request review from a member of a different team.
  • Have the Pivotal acceptance criteria been met for this change?

References

Screenshots

Screen Shot 2019-10-01 at 4 02 49 PM

Screen Shot 2019-10-01 at 4 02 42 PM

@codecov

This comment has been minimized.

Copy link

commented Oct 1, 2019

Codecov Report

Merging #2757 into master will increase coverage by 0.2%.
The diff coverage is 84.3%.

@@           Coverage Diff            @@
##           master   #2757     +/-   ##
========================================
+ Coverage    57.6%   57.7%   +0.2%     
========================================
  Files         278     280      +2     
  Lines       12567   12609     +42     
========================================
+ Hits         7233    7270     +37     
- Misses       4586    4591      +5     
  Partials      748     748
Impacted Files Coverage Δ
pkg/handlers/adminapi/electronic_orders.go 37.9% <0%> (-0.5%) ⬇️
pkg/handlers/adminapi/api.go 0% <0%> (ø) ⬆️
pkg/handlers/adminapi/offices.go 92% <100%> (+0.4%) ⬆️
pkg/handlers/adminapi/admin_users.go 92.6% <100%> (+0.3%) ⬆️
...g/services/office_user/office_user_list_fetcher.go 100% <100%> (ø) ⬆️
.../electronic_order/electronic_order_list_fetcher.go 100% <100%> (ø) ⬆️
pkg/handlers/adminapi/upload.go 100% <100%> (ø)
pkg/services/admin_user/admin_user_list_fetcher.go 100% <100%> (ø) ⬆️
pkg/services/office/office_list_fetcher.go 100% <100%> (ø) ⬆️
pkg/handlers/adminapi/office_users.go 91% <100%> (+0.2%) ⬆️
... and 6 more
Copy link
Contributor

left a comment

We ought to be able to test this on dev by enabling S3 storage backend with STORAGE_BACKEND=s3 and then uploading a file. That file will end up in the s3 bucket s3://transcom-ppp-app-devlocal-us-west-2/USERNAME where USERNAME is the echo $USER from your laptop. I think yours is s3://transcom-ppp-app-devlocal-us-west-2/karalouie.

The file I'd like to upload is the EICAR Test File specifically this one: http://www.eicar.org/download/eicar.com.txt. That file ought to trigger as INFECTED by the AV system and will spit out an Upload ID. Then we take that ID from Slack and drop it in your search and see it work.

If that all works then I'd love to do this in Experimental and perform the same experiment.

Copy link
Contributor

left a comment

One small thing with a duplicated function, but otherwise this is looking good. Thanks @kahlouie

func (p *Builder) FetchWithAssociations(model interface{}, filters []services.QueryFilter, associations services.QueryAssociations) error {
t := reflect.TypeOf(model)
if t.Kind() != reflect.Ptr {
return errors.New(fetchOneReflectionMessage)
}
t = t.Elem()
if t.Kind() != reflect.Slice {
return errors.New(fetchManyReflectionMessage)
}
t = t.Elem()
if t.Kind() != reflect.Struct {
return errors.New(fetchManyReflectionMessage)
}
query := p.db.Q()
query, err := filteredQuery(query, filters, t)
if err != nil {
return err
}
err = associatedQuery(query, associations, model)
if err != nil {
return err
}
return nil
}

Comment for lines 175  – 200

This comment has been minimized.

Copy link
@garrettqmartin8

garrettqmartin8 Oct 2, 2019

Contributor

I believe this is a duplicate of QueryForAssociations in the same file.

On another note:

Could both the function you wrote and the QueryForAssociations be removed in favor of passing associations services.QueryAssociations into FetchMany? This way, we don't keep expanding the surface area of the API. I don't think this should hold up the PR since @Ryan-Koch and I have been thinking of ways to make the query builder API more intuitive, but just want to get my thoughts around that on your radar.

This comment has been minimized.

Copy link
@kahlouie

kahlouie Oct 2, 2019

Author Contributor

Ooh, that's a great idea. I intended to only fetch one with associations, but I'll look into folding it into FetchMany

This comment has been minimized.

Copy link
@garrettqmartin8

garrettqmartin8 Oct 2, 2019

Contributor

Ah yeah, you could also try folding it into FetchOne since you're just querying for one record.

This comment has been minimized.

Copy link
@kahlouie

kahlouie Oct 4, 2019

Author Contributor

@garrettqmartin8 I'm going to hold off folding it into fetch one only because there are so many instances I'd need to change to get it to work. This is blocking another story. If it's cool with you, I'd like to merge it in as it is now and update that separately.

This comment has been minimized.

Copy link
@garrettqmartin8

garrettqmartin8 Oct 4, 2019

Contributor

Totally cool, I'll revisit the API design of the query builder in the next couple of sprints. Thanks for giving it a go!

@kahlouie kahlouie force-pushed the kl-admin-uploads branch from 96018ee to c95a821 Oct 2, 2019
Copy link
Contributor

left a comment

🚀 - This works great.

@kahlouie kahlouie force-pushed the kl-admin-uploads branch from 15853c0 to eb0f95e Oct 4, 2019
@kahlouie kahlouie merged commit f68e527 into master Oct 4, 2019
18 checks passed
18 checks passed
ci/circleci: acceptance_tests_experimental Your tests passed on CircleCI!
Details
ci/circleci: acceptance_tests_local Your tests passed on CircleCI!
Details
ci/circleci: acceptance_tests_staging Your tests passed on CircleCI!
Details
ci/circleci: build_app Your tests passed on CircleCI!
Details
ci/circleci: build_migrations Your tests passed on CircleCI!
Details
ci/circleci: build_storybook_app Your tests passed on CircleCI!
Details
ci/circleci: build_tasks Your tests passed on CircleCI!
Details
ci/circleci: build_tools Your tests passed on CircleCI!
Details
ci/circleci: check_generated_code Your tests passed on CircleCI!
Details
ci/circleci: client_test Your tests passed on CircleCI!
Details
ci/circleci: integration_tests Your tests passed on CircleCI!
Details
ci/circleci: pre_deps_golang Your tests passed on CircleCI!
Details
ci/circleci: pre_deps_yarn Your tests passed on CircleCI!
Details
ci/circleci: pre_test Your tests passed on CircleCI!
Details
ci/circleci: server_test Your tests passed on CircleCI!
Details
ci/circleci: server_test_coverage Your tests passed on CircleCI!
Details
codecov/patch/go 84.3% of diff hit (target 57.4%)
Details
codecov/project/go 57.5% (+0.2%) compared to 20214bc
Details
@kahlouie kahlouie deleted the kl-admin-uploads branch Oct 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.