Adding inital data comparison #5206
Draft
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Not linted, but it is tested. Want to get other people's eyes on it.
Contributor
|
This pull request is not up to date with main. Please merge main into this brach or rebase this branch onto main. This PR should not be approved until all status checks pass. If you see this message, please rerun all status checks before merging. |
Contributor
|
Terraform plan for meta No changes. Your infrastructure matches the configuration.📝 Plan generated in Pull Request Checks #157 |
Contributor
|
Terraform plan for dev Plan: 3 to add, 5 to change, 3 to destroy.Terraform used the selected providers to generate the following execution
plan. Resource actions are indicated with the following symbols:
!~ update in-place
-/+ destroy and then create replacement
Terraform will perform the following actions:
# module.dev.cloudfoundry_network_policy.app-network-policy will be updated in-place
!~ resource "cloudfoundry_network_policy" "app-network-policy" {
!~ policies = [
!~ {
!~ destination_app = "830bfc16-6865-4644-a3f3-5a1a69d6ec5f" -> (known after apply)
# (3 unchanged attributes hidden)
},
!~ {
!~ destination_app = "830bfc16-6865-4644-a3f3-5a1a69d6ec5f" -> (known after apply)
# (3 unchanged attributes hidden)
},
# (1 unchanged element hidden)
]
}
# module.dev.cloudfoundry_network_policy.clamav-network-policy will be updated in-place
!~ resource "cloudfoundry_network_policy" "clamav-network-policy" {
!~ policies = [
!~ {
!~ source_app = "830bfc16-6865-4644-a3f3-5a1a69d6ec5f" -> (known after apply)
# (3 unchanged attributes hidden)
},
!~ {
!~ source_app = "d9fa2027-96c9-4f98-bdb8-13809c6f569a" -> (known after apply)
# (3 unchanged attributes hidden)
},
]
}
# module.dev.cloudfoundry_network_policy.scanner-network-policy will be updated in-place
!~ resource "cloudfoundry_network_policy" "scanner-network-policy" {
!~ policies = [
!~ {
!~ destination_app = "d9fa2027-96c9-4f98-bdb8-13809c6f569a" -> (known after apply)
# (3 unchanged attributes hidden)
},
# (1 unchanged element hidden)
]
}
# module.dev.module.clamav.cloudfoundry_app.clamav_api must be replaced
-/+ resource "cloudfoundry_app" "clamav_api" {
+ buildpacks = (known after apply)
!~ created_at = "2025-08-12T17:37:02Z" -> (known after apply)
!~ docker_image = "ghcr.io/gsa-tts/fac/clamav@sha256:f0b490065d736a7c4151744e04683760418fe7a43ba9e14102b162305d94966a" -> "ghcr.io/gsa-tts/fac/clamav@sha256:3c7acdf614fba2604a5aaf4a015c803a02a6bb79cd68f1636577557a0a9384bf"
!~ enable_ssh = false -> (known after apply)
!~ health_check_type = "port" -> (known after apply)
!~ id = "************************************" -> (known after apply)
!~ log_rate_limit_per_second = "-1" -> (known after apply)
name = "fac-av-dev"
!~ readiness_health_check_type = "process" -> (known after apply)
!~ routes = [
- {
- protocol = "http1" -> null
- route = "fac-av-dev.apps.internal" -> null
},
] -> (known after apply)
+ service_bindings = (known after apply) # forces replacement
!~ stack = null -> (known after apply)
!~ updated_at = "2025-08-12T17:37:09Z" -> (known after apply)
# (8 unchanged attributes hidden)
}
# module.dev.module.cors.null_resource.cors_header must be replaced
-/+ resource "null_resource" "cors_header" {
!~ id = "*******************" -> (known after apply)
!~ triggers = { # forces replacement
!~ "always_run" = "2025-08-22T18:58:30Z" -> (known after apply)
}
}
# module.dev.module.file_scanner_clamav.cloudfoundry_app.clamav_api must be replaced
-/+ resource "cloudfoundry_app" "clamav_api" {
+ buildpacks = (known after apply)
!~ created_at = "2025-08-12T17:39:11Z" -> (known after apply)
!~ docker_image = "ghcr.io/gsa-tts/fac/clamav@sha256:f0b490065d736a7c4151744e04683760418fe7a43ba9e14102b162305d94966a" -> "ghcr.io/gsa-tts/fac/clamav@sha256:3c7acdf614fba2604a5aaf4a015c803a02a6bb79cd68f1636577557a0a9384bf"
!~ enable_ssh = false -> (known after apply)
!~ health_check_type = "port" -> (known after apply)
!~ id = "************************************" -> (known after apply)
!~ log_rate_limit_per_second = "-1" -> (known after apply)
name = "fac-av-dev-fs"
!~ readiness_health_check_type = "process" -> (known after apply)
!~ routes = [
- {
- protocol = "http1" -> null
- route = "fac-av-dev-fs.apps.internal" -> null
},
] -> (known after apply)
+ service_bindings = (known after apply) # forces replacement
!~ stack = null -> (known after apply)
!~ updated_at = "2025-08-12T17:39:17Z" -> (known after apply)
# (8 unchanged attributes hidden)
}
# module.dev.module.clamav.module.route.cloudfoundry_route.app_route will be updated in-place
!~ resource "cloudfoundry_route" "app_route" {
!~ destinations = [
- {
- app_id = "830bfc16-6865-4644-a3f3-5a1a69d6ec5f" -> null
- app_process_type = "web" -> null
- id = "e72b846c-1652-4ba8-ab3e-03ed29621a63" -> null
- port = 8080 -> null
- protocol = "http1" -> null
},
+ {
+ app_id = (known after apply)
+ app_process_type = (known after apply)
+ id = (known after apply)
+ port = (known after apply)
+ protocol = (known after apply)
},
]
id = "877bbc1f-e036-4a96-b7a3-70cff8c35c3c"
!~ updated_at = "2025-08-12T17:37:10Z" -> (known after apply)
# (6 unchanged attributes hidden)
}
# module.dev.module.file_scanner_clamav.module.route.cloudfoundry_route.app_route will be updated in-place
!~ resource "cloudfoundry_route" "app_route" {
!~ destinations = [
- {
- app_id = "d9fa2027-96c9-4f98-bdb8-13809c6f569a" -> null
- app_process_type = "web" -> null
- id = "ca09f24f-42c2-4f01-b2ce-d9dcec58286d" -> null
- port = 8080 -> null
- protocol = "http1" -> null
},
+ {
+ app_id = (known after apply)
+ app_process_type = (known after apply)
+ id = (known after apply)
+ port = (known after apply)
+ protocol = (known after apply)
},
]
id = "18f0f97d-c1a1-4628-8700-70fe47c19114"
!~ updated_at = "2025-08-22T18:59:03Z" -> (known after apply)
# (6 unchanged attributes hidden)
}
Plan: 3 to add, 5 to change, 3 to destroy.📝 Plan generated in Pull Request Checks #157 |
Visit http://localhost:8000/audit/compare/2023-12-GSAFAC-0000058119/2023-12-GSAFAC-0000065436 to see a pair of audits compared. These are a "resubmission." For two that are very different: http://localhost:8000/audit/compare/2023-12-GSAFAC-0000058119/2023-09-GSAFAC-0000016690 This assumes *full* data loaded. No need to truncate.
Contributor
|
This pull request is not up to date with main. Please merge main into this brach or rebase this branch onto main. This PR should not be approved until all status checks pass. If you see this message, please rerun all status checks before merging. |
Contributor
|
This pull request is not up to date with main. Please merge main into this brach or rebase this branch onto main. This PR should not be approved until all status checks pass. If you see this message, please rerun all status checks before merging. |
Adding more info to what comes back, so that we can present it better. More consistent across categories of difference, for ease of rendering. Improved layout.
Contributor
|
This pull request is not up to date with main. Please merge main into this brach or rebase this branch onto main. This PR should not be approved until all status checks pass. If you see this message, please rerun all status checks before merging. |
This would be what it looks like if we only show what changed. The page gets much shorter. And, perhaps, more understandable, because we are not showing things that did not change. Less to parse.
Contributor
|
This pull request is not up to date with main. Please merge main into this brach or rebase this branch onto main. This PR should not be approved until all status checks pass. If you see this message, please rerun all status checks before merging. |
More tests. The "compare_with_previous" code can serve as test code for resubmission generation, in a way. This fixes how we initiate a resubmission. It eliminates most data being copied over. We should largely end up with an empty audit, save for some metadata. Correctly yields differences.
Contributor
|
This pull request is not up to date with main. Please merge main into this brach or rebase this branch onto main. This PR should not be approved until all status checks pass. If you see this message, please rerun all status checks before merging. |
Not sure where it happens. However, it cannot be allowed to happen. When we do a resub, it cannot modify the original.
Contributor
|
This pull request is not up to date with main. Please merge main into this brach or rebase this branch onto main. This PR should not be approved until all status checks pass. If you see this message, please rerun all status checks before merging. |
added 11 commits
August 22, 2025 09:48
I think. With many, many assertions to make sure.
Need more tests around this, but it looks like it is now doing the right things with the set diff/intersections.
Pulls PDF reports from Minio and checks if the SHA and length are equal. If not, it reports them as different. Needs to be tested in CGov. Uses interfaces that "should" work in CGov.
This includes a test where a single character (a few bits) are changed. We detect it. This is good.
This updates the view to accept only one audit. It also implements the access controls as discussed, with minor changes. It is unclear whether the diff should *always* be available, even after submission. In this way, an oversight official can refer to the diff page, and the owners/participants on the audit will still be able to see it. It will continue to prevent people *not* associated with the audit from seeing the diff, so that seems OK.
I am stuck on why I am getting access denied errors in the view tests.
The issue was with how the test DB was being constructed. There were invariants being broken (?!), and as a result, I had reports with duplicate report IDs. This broke selection logic in the code. Once all of that was figured out, the view tests were fine. The issue was entirely with test setup/data construction.
If authenticated as a Fed, you can see any resubmission.
It needs a bunch of help still. Still have not tablefied the resubmission comparison page. Also have questions about how I integrated it into the checklist.
Also, reporting errors back to the user.
Contributor
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
From ticket: #5199
problem
We want to provide an initial/v1 overview of "what changed?" between two audits in a resubmission. We want to do this in two places:
solution
This PR introduces:
screen grabs
New checklist item
When a resubmission is active, there is a new element to the resubmission checklist.
Comparison page
This is a representative comparison page.
testing
There are unit tests and view tests integrated into this PR. Those tests insert data into a mock DB to test authentication/identity and data-oriented elements of the comparison tool. The view has tests to make sure access controls are enforced, and rudimentary checks to make sure data comes back via the HTML view.
To test manually:
2023-06-GSAFAC-0000000697 is intentionally rolled back from a
DISSEMINATEDtoIN_PROGRESSstate, so that there is at least one audit in the test set that is not yet complete. This audit can be used to view/test the audit checklist.http://localhost:8000/audit/submission-progress/2023-06-GSAFAC-0000376788
PR Checklist: Submitter
maininto your branch shortly before creating the PR. (You should also be mergingmaininto your branch regularly during development.)git status | grep migrations. If there are any results, you probably need to add them to the branch for the PR. Your PR should have only one new migration file for each of the component apps, except in rare circumstances; you may need to delete some and re-runpython manage.py makemigrationsto reduce the number to one. (Also, unless in exceptional circumstances, your PR should not delete any migration files.)PR Checklist: Reviewer
make docker-clean; make docker-first-run && docker compose up; then rundocker compose exec web /bin/bash -c "python manage.py test"The larger the PR, the stricter we should be about these points.
Pre Merge Checklist: Merger
-/+ resource "null_resource" "cors_header"should be destroying and recreating its self and~ resource "cloudfoundry_app" "clamav_api"might be updating itssha256for thefac-file-scannerandfac-av-${ENV}by default.main.