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

Backend/755/rename reports #1160

Merged
merged 25 commits into from
Aug 17, 2021
Merged

Conversation

riatzukiza
Copy link

@riatzukiza riatzukiza commented Jul 28, 2021

Summary of Changes

resolves #755

How to Test

  1. locally, run docker system prune -a to start from a clean slate
  2. Starting from raft-tdp-main, spin up the backend and make a new user and upload a few data files
  3. open the django admin panel, check that the files were added
  4. switch to this branch, run docker-compose run web sh -c 'python manage.py migrate'
  5. run docker-compose restart web to only restart the django container
  6. open the DAC and check that the records are all called DataFiles
  7. navigate to frontend reports page to see that the file still shows up in the same place.

Deliverable 1: Accepted Features

Performance Standard(s): At the beginning of each sprint, the Product Owner and development team will collaborate to define a set of user stories to be completed during the sprint. Acceptance criteria for each story will also be defined. The development team will deliver code and functionality to satisfy these user stories.

Acceptable Quality Level: Delivered code meets the acceptance criteria for each user story. Incomplete stories will be assessed and considered for inclusion in the next sprint.

  • Look up the acceptance criteria in the related issue; paste ACs below in checklist format.
  • Check against the criteria:

As facilitator/product manager, @kniz-raft will decide if ACs are met from Raft's perspective.

Deliverable 2: Tested Code

Performance Standard(s): Code delivered under the order must have substantial test code coverage. Version-controlled HHS GitHub repository of code that comprises products that will remain in the government domain.

Acceptable Quality Level: Minimum of 90% test coverage of all code. All areas of code are meaningfully tested.

  • Are all areas of code introduced in this PR meaningfully tested?
    • If this PR introduces backend code changes, are they meaningfully tested?
    • If this PR introduces frontend code changes, are they meaningfully tested?
  • Are code coverage minimums met?

Deliverable 3: Properly Styled Code

Performance Standard(s): GSA 18F Front- End Guide

Acceptable Quality Level: 0 linting errors and 0 warnings

  • Are backend code style checks passing on CircleCI?
  • Are frontend code style checks passing on CircleCI?
  • Does this PR change any linting or CI settings?

Deliverable 4: Accessible

Performance Standard(s): Web Content Accessibility Guidelines 2.1 AA standards

Acceptable Quality Level: 0 errors reported using an automated scanner and 0 errors reported in manual testing

Deliverable 5: Deployed

Performance Standard(s): Code must successfully build and deploy into the staging environment.

Acceptable Quality Level: Successful build with a single command

NOTE: until we have a proper staging environment this may not be satisfiable prior to merging

  • Was the code successfully deployed via automated CircleCI process to development on Cloud.gov?

Deliverable 6: Documented

Performance Standard(s): Summary of user stories completed every two weeks. All dependencies are listed and the licenses are documented. Major functionality in the software/source code is documented, including system diagram. Individual methods are documented inline in a format that permits the use of tools such as JSDoc. All non-inherited 800-53 system security controls are documented in the Open Control or OSCAL format and HHS Section 508 Product Assessment Template (PAT) are updated as appropriate.

Acceptable Quality Level: Combination of manual review and automated testing, if available

  • If this PR introduces backend code, is that code documented both inline and overall?
  • If this PR introduces frontend code, is that code documented both inline and overall?
  • If this PR introduces dependencies, are their licenses documented?

Deliverable 7: Secure

Performance Standard(s): Open Web Application Security Project (OWASP) Application Security Verification Standard 3.0

Acceptable Quality Level: Code submitted must be free of medium- and high-level static and dynamic security vulnerabilities

  • Does the OWASP Scan pass on CircleCI?
  • Do manual code review and manual testing detect any security issues?

@@ -13,10 +16,12 @@ class Migration(migrations.Migration):
("stts", "0002_auto_20200923_1809"),
migrations.swappable_dependency(settings.AUTH_USER_MODEL),
]
replaces = [('reports','0001_initial')]

Choose a reason for hiding this comment

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

Great find here! This looks like a great way to both retain the history and ensure a stable migration path on deployment.

@riatzukiza riatzukiza added the raft review This issue is ready for raft review label Jul 29, 2021
@riatzukiza riatzukiza self-assigned this Jul 29, 2021
@riatzukiza riatzukiza marked this pull request as ready for review July 29, 2021 19:54
Copy link

@jtwillis92 jtwillis92 left a comment

Choose a reason for hiding this comment

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

I had some trouble testing this locally, but I think its due to the fact that the postgres contents are wiped on docker restarts and the Django server needs to be restarted to load in the code changes. I was able to run through the listed steps and confirmed the table was updated, but when I loaded the Django admin the server was still running code pointing to ReportFile so I got an error trying to view the migrated models. It may be easier to test that path on the deployed site, but there are some conflicts on this PR which prevents it from getting the latest changes from master related to S3 configurations and the deployed sites no longer have AWS credentials set under the old environment variable names.

tdrs-backend/tdpservice/conftest.py Outdated Show resolved Hide resolved
@@ -29,7 +29,7 @@
path("auth_check", AuthorizationCheck.as_view(), name="authorization-check"),
path("", include("tdpservice.users.urls")),
path("stts/", include("tdpservice.stts.urls")),
path("reports/", include("tdpservice.reports.urls")),
path("data_files/", include("tdpservice.data_files.urls")),

Choose a reason for hiding this comment

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

It looks like we'll need to at least update the frontend Redux actions to make axios calls to this URL instead to prevent 404 errors on the client side.

Renaming the rest of the frontend files would be out of scope for this issue and should be tracked with #1132

@codecov
Copy link

codecov bot commented Aug 4, 2021

Codecov Report

Merging #1160 (1606c1c) into raft-tdp-main (c36edb9) will not change coverage.
The diff coverage is 98.88%.

Impacted file tree graph

@@              Coverage Diff               @@
##           raft-tdp-main    #1160   +/-   ##
==============================================
  Coverage          98.14%   98.14%           
==============================================
  Files                 38       38           
  Lines                917      917           
  Branches              42       42           
==============================================
  Hits                 900      900           
  Misses                12       12           
  Partials               5        5           
Flag Coverage Δ
dev-backend 98.14% <98.88%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
tdrs-backend/tdpservice/data_files/validators.py 84.61% <ø> (ø)
tdrs-backend/tdpservice/settings/common.py 100.00% <ø> (ø)
tdrs-backend/tdpservice/urls.py 100.00% <ø> (ø)
tdrs-backend/tdpservice/data_files/errors.py 66.66% <66.66%> (ø)
tdrs-backend/tdpservice/core/views.py 100.00% <100.00%> (ø)
tdrs-backend/tdpservice/data_files/admin.py 100.00% <100.00%> (ø)
tdrs-backend/tdpservice/data_files/apps.py 100.00% <100.00%> (ø)
tdrs-backend/tdpservice/data_files/models.py 100.00% <100.00%> (ø)
tdrs-backend/tdpservice/data_files/serializers.py 100.00% <100.00%> (ø)
tdrs-backend/tdpservice/data_files/urls.py 100.00% <100.00%> (ø)
... and 7 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 c36edb9...1606c1c. Read the comment docs.

Copy link

@jtwillis92 jtwillis92 left a comment

Choose a reason for hiding this comment

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

Tested this successfully from the backend perspective, but I did have to use the command: docker-compose run web sh -c 'python manage.py migrate' instead of what is listed in the PR description.

However, this causes a breaking change on the frontend because of the URL change:
404-reports

@riatzukiza
Copy link
Author

Tested this successfully from the backend perspective, but I did have to use the command: docker-compose run web sh -c 'python manage.py migrate' instead of what is listed in the PR description.

However, this causes a breaking change on the frontend because of the URL change:
404-reports

@jtwillis92 You mentioned in another comment to address these issues in another pr? #1132? I understand not wanting to merge this till then, so what should we do?

@jtwillis92
Copy link

@jtwillis92 You mentioned in another comment to address these issues in another pr? #1132? I understand not wanting to merge this till then, so what should we do?

My apologies, I meant in that other comment to address renaming the rest of the frontend code as part of a follow on PR. For this PR we should be able to move it forward by only updating the URLs used in tdrs-frontend/src/actions/reports.js.

@riatzukiza riatzukiza requested a review from jtwillis92 August 6, 2021 13:28
@andrew-jameson andrew-jameson changed the title [WIP]Backend/755/rename reports Backend/755/rename reports Aug 6, 2021
Copy link

@jtwillis92 jtwillis92 left a comment

Choose a reason for hiding this comment

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

LGTM, tested locally with no issues on either side!

dependencies = [
('reports', '0002_auto_20201215_1932'),
('data_files', '0002_auto_20201215_1932'),

Choose a reason for hiding this comment

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

Not directly related to this PR or anything; we should probably rename the more unreadable migration files at some point.

Copy link

@jorgegonzalez jorgegonzalez left a comment

Choose a reason for hiding this comment

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

LGTM, tested this locally with no issues 👍

…entry_content_type.py

Co-authored-by: John Willis <jtwillis92@gmail.com>
@riatzukiza riatzukiza added QASP Review and removed raft review This issue is ready for raft review labels Aug 16, 2021
@ADPennington
Copy link
Collaborator

@riatzukiza test results 👍🏾

  • submitting test file on raft-tdp-main branch. confirmed it shows up in DAC
    1160p1
    1160p2

  • after switching to PR branch and applying migration
    1160p3
    1160p4

Copy link
Collaborator

@ADPennington ADPennington left a comment

Choose a reason for hiding this comment

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

@riatzukiza thanks 👍🏾 @abottoms-coder this is ready to merge. note the reference to a codecov finding below worth further research re: if this is a problem.

Deliverable 1: Accepted Features

Performance Standard(s): At the beginning of each sprint, the Product Owner and development team will collaborate to define a set of user stories to be completed during the sprint. Acceptance criteria for each story will also be defined. The development team will deliver code and functionality to satisfy these user stories.

Acceptable Quality Level: Delivered code meets the acceptance criteria for each user story. Incomplete stories will be assessed and considered for inclusion in the next sprint.

  • Look up the acceptance criteria in the related issue; paste ACs below in checklist format.
  • Check against the criteria:
    • All references in code to reports are changed to data_files
    • Database tables and fields are updated so any reference to report or reports are changed to data_file and data_files respectively.
    • Testing Checklist has been run and all tests pass

As Product Owner, @lfrohlich will decide if ACs are met.

Deliverable 2: Tested Code

Performance Standard(s): Code delivered under the order must have substantial test code coverage. Version-controlled HHS GitHub repository of code that comprises products that will remain in the government domain.

Acceptable Quality Level: Minimum of 90% test coverage of all code. All areas of code are meaningfully tested.

Deliverable 3: Properly Styled Code

Performance Standard(s): GSA 18F Front- End Guide

Acceptable Quality Level: 0 linting errors and 0 warnings

  • Are backend code style checks passing on CircleCI?
  • Are frontend code style checks passing on CircleCI?

Deliverable 4: Accessible

n/a

Deliverable 5: Deployed

Performance Standard(s): Code must successfully build and deploy into the staging environment.

Acceptable Quality Level: Successful build with a single command

  • Was the code successfully deployed via automated CircleCI process to development on Cloud.gov?

Deliverable 6: Documented

Performance Standard(s): Summary of user stories completed every two weeks. All dependencies are listed and the licenses are documented. Major functionality in the software/source code is documented, including system diagram. Individual methods are documented inline in a format that permits the use of tools such as JSDoc. All non-inherited 800-53 system security controls are documented in the Open Control or OSCAL format and HHS Section 508 Product Assessment Template (PAT) are updated as appropriate.

Acceptable Quality Level: Combination of manual review and automated testing, if available

  • If this PR introduces backend code, is that code documented both inline and overall?

Deliverable 7: Secure

Performance Standard(s): Open Web Application Security Project (OWASP) Application Security Verification Standard 3.0

Acceptable Quality Level: Code submitted must be free of medium- and high-level static and dynamic security vulnerabilities

  • Does the OWASP Scan pass on CircleCI?
  • Do manual code review and manual testing detect any security issues?
    no new issues

@andrew-jameson andrew-jameson merged commit 17b0230 into raft-tdp-main Aug 17, 2021
@andrew-jameson andrew-jameson deleted the backend/755/rename_reports branch August 17, 2021 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Backend] Change reports to data files
5 participants