3399 controlled access fra data files page #3430

raftmsohani · 2025-01-16T19:11:16Z

Summary of Changes

Provide a brief summary of changes
Pull request closes #3399 _

How to Test

List the steps to test the PR

start the backend and frontend
login to admin as superuser
make sure you have datafiles for testing. change a couple of sections to FRA sections
check that without fra_access, you user cannot see the files in FRA sections
add fra access by adding the following to feature flag section: {"fra_access": true}
check that now you can see the FRA files only

Demo GIF(s) and screenshots for testing procedure

Deliverables

More details on how deliverables herein are assessed included here.

Deliverable 1: Accepted Features

Checklist of ACs:

[insert ACs here]
lfrohlich and/or adpennington confirmed that ACs are met.

Deliverable 2: Tested Code

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?
- Frontend coverage: [insert coverage %] (see CodeCov Report comment in PR)
- Backend coverage: [insert coverage %] (see CodeCov Report comment in PR)

Deliverable 3: Properly Styled Code

Are backend code style checks passing on CircleCI?
Are frontend code style checks passing on CircleCI?
Are code maintainability principles being followed?

Deliverable 4: Accessible

Does this PR complete the epic?
Are links included to any other gov-approved PRs associated with epic?
Does PR include documentation for Raft's a11y review?
Did automated and manual testing with iamjolly and ttran-hub using Accessibility Insights reveal any errors introduced in this PR?

Deliverable 5: Deployed

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

Deliverable 6: Documented

Does this PR provide background for why coding decisions were made?
If this PR introduces backend code, is that code easy to understand and sufficiently documented, both inline and overall?
If this PR introduces frontend code, is that code easy to understand and sufficiently documented, both inline and overall?
If this PR introduces dependencies, are their licenses documented?
Can reviewer explain and take ownership of these elements presented in this code review?

Deliverable 7: Secure

Does the OWASP Scan pass on CircleCI?
Do manual code review and manual testing detect any new security issues?
If new issues detected, is investigation and/or remediation plan documented?

Deliverable 8: User Research

Research product(s) clearly articulate(s):

the purpose of the research
methods used to conduct the research
who participated in the research
what was tested and how
impact of research on TDP
(if applicable) final design mockups produced for TDP development

…led-access-FRA-Data-Files-page-3

codecov · 2025-01-21T04:42:16Z

Codecov Report

Attention: Patch coverage is 66.66667% with 25 lines in your changes missing coverage. Please review.

Project coverage is 91.26%. Comparing base (099b230) to head (2b0e4be).
Report is 4 commits behind head on develop.

Files with missing lines	Patch %	Lines
tdrs-backend/tdpservice/data_files/admin/admin.py	48.00%	13 Missing ⚠️
tdrs-backend/tdpservice/data_files/views.py	16.66%	4 Missing and 1 partial ⚠️
tdrs-backend/tdpservice/users/admin.py	28.57%	5 Missing ⚠️
tdrs-backend/tdpservice/data_files/models.py	90.47%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #3430      +/-   ##
===========================================
- Coverage    91.43%   91.26%   -0.17%     
===========================================
  Files          300      302       +2     
  Lines         8647     8720      +73     
  Branches       640      650      +10     
===========================================
+ Hits          7906     7958      +52     
- Misses         621      642      +21     
  Partials       120      120

Flag	Coverage Δ
dev-backend	`91.10% <66.66%> (-0.20%)`	⬇️
dev-frontend	`92.44% <ø> (ø)`

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

Files with missing lines	Coverage Δ
...ta_files/migrations/0017_alter_datafile_section.py	`100.00% <100.00%> (ø)`
tdrs-backend/tdpservice/data_files/serializers.py	`96.55% <100.00%> (+0.25%)`	⬆️
...ervice/users/migrations/0042_user_feature_flags.py	`100.00% <100.00%> (ø)`
tdrs-backend/tdpservice/users/models.py	`93.93% <100.00%> (+2.36%)`	⬆️
tdrs-backend/tdpservice/data_files/models.py	`87.64% <90.47%> (+0.29%)`	⬆️
tdrs-backend/tdpservice/data_files/views.py	`85.21% <16.66%> (-3.78%)`	⬇️
tdrs-backend/tdpservice/users/admin.py	`72.72% <28.57%> (-8.76%)`	⬇️
tdrs-backend/tdpservice/data_files/admin/admin.py	`66.34% <48.00%> (-4.54%)`	⬇️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4849833...2b0e4be. Read the comment docs.

andrew-jameson · 2025-01-21T15:27:10Z

Hey Mo, can you update testing steps here? Functionally it's looking good

tdrs-backend/tdpservice/users/migrations/0042_user_feature_flags.py

…gs.py Co-authored-by: Andrew <[email protected]>

elipe17 · 2025-01-22T14:58:40Z

tdrs-backend/tdpservice/data_files/test/test_api.py

+    def test_create_data_file_fra(self, api_client, data_file_data, user):
+        """Test ability to create data file metadata registry."""
+        response = self.post_data_file_fra(api_client, data_file_data)
+        from rest_framework.exceptions import ErrorDetail


Is there a reason this lives in the function instead of at the module level?

not really, corrected it

elipe17 · 2025-01-22T14:59:59Z

tdrs-backend/tdpservice/users/admin.py

@@ -19,6 +21,9 @@ class Meta:
        exclude = ['password', 'user_permissions']
        readonly_fields = ['last_login', 'date_joined', 'login_gov_uuid', 'hhs_id', 'access_request']

+    def __init__(self, *args, **kwargs):


Is this stylistic/for completeness, or was this required?

byproduct of an approach I was taking, can be removed

tdrs-backend/tdpservice/users/test/test_models.py

elipe17 · 2025-01-22T15:04:37Z

tdrs-backend/tdpservice/users/test/test_models.py

+    client = Client()
+    client.force_login(user)
+
+    # Need a datafile, with a section that is not in the FRA_SECTION_LIST


This comment is confusing since the test is using an FRA section immediately below it even though the comment says not to.

elipe17

Left some comments and questions

elipe17 · 2025-01-27T15:18:23Z

tdrs-backend/tdpservice/users/models.py

+        default=dict,
+        help_text="Feature flags for this data file.",
+        null=True,
+        blank=True,


If we keep null and blank set to True, the property has_fra_access should do a null check on self.feature_flags before checking for the key. I was able to leave the form field blank in the admin and cause an exception to be raised when navigating to the data files page. Could we instead make them both false since we do have the default set as dict to avoid worrying about checks?

Unfortunately, there is an internal check for {} in JSONField. The only way to bypass is to override JSONField, but I decided to check for null or empty dict during cleaning which is simple

Also moved null out

elipe17

Checkout my comment

elipe17 · 2025-01-27T15:27:53Z

@raftmsohani this PR is also generating another migration on top of the two existing ones when I start it locally.\

raftmsohani · 2025-01-28T15:18:37Z

@raftmsohani this PR is also generating another migration on top of the two existing ones when I start it locally.\

fixed

jtimpe

Working for the most part. I do have one suggestion i think would be worth exploring

jtimpe · 2025-01-29T15:04:56Z

tdrs-backend/tdpservice/data_files/admin/admin.py

+            filtered_for_fra = qs.exclude(section__in=FRA_SECTION_LIST)
+            return filtered_for_fra
+        else:
+            return qs.filter(section__in=FRA_SECTION_LIST)


not sure if this is intended, but setting fra_access=True means that the data files page filters down to only FRA files. you have to set fra_access=False to see other file types. maybe we should have a "both" option for admins and other users so that they don't have to switch the feature flag to find files in the admin

raftmsohani · 2025-01-29T20:19:20Z

tdrs-backend/tdpservice/data_files/admin/admin.py

@@ -37,6 +37,16 @@ class Media:

    actions = ['reparse']

+    def get_queryset(self, request):


if user doesn't have FRA access or is Admin, then FRA files will be filtered

raftmsohani · 2025-01-29T20:22:08Z

tdrs-backend/tdpservice/data_files/admin/admin.py

@@ -135,6 +145,28 @@ def queryset(self, request, queryset):
            else:
                return queryset

+    class FRA_AccessFilter(admin.SimpleListFilter):


This filter will appear if user has FRA access and is Admin

jtimpe

Looks good

raftmsohani and others added 11 commits January 7, 2025 15:49

added new grouping to model enum

5ffa00c

added tests

c9d2480

added chack for FRA file

a8dd63d

Added API test

b9f8331

Update serializers.py

2027bad

Merge branch 'develop' into 3397-FRA-report-types-dataFile-model

0cc815c

skip parsing

9f93409

fix linting

e0859a7

Merge branch '3397-FRA-report-types-dataFile-model' into 3399-control…

486b19a

…led-access-FRA-Data-Files-page-3

added fra feature flag and filtering

6f18151

added functional test

75405a9

raftmsohani self-assigned this Jan 16, 2025

raftmsohani added 3 commits January 16, 2025 14:12

task file

cd1a074

task file

e1a9651

task file

8e75ee7

raftmsohani requested review from andrew-jameson, jtimpe and elipe17 January 16, 2025 19:15

raftmsohani added raft review This issue is ready for raft review backend labels Jan 16, 2025

Merge branch 'develop' into 3399-controlled-access-FRA-Data-Files-page-3

05c20ce

andrew-jameson added the Deploy with CircleCI-raft Deploy to https://tdp-frontend-raft.app.cloud.gov through CircleCI label Jan 17, 2025

raftmsohani and others added 4 commits January 17, 2025 11:32

fixed failing e2e

fc8e14a

increased timeout

54d3b7e

fix problems with e2e

dc3cd9d

Merge branch 'develop' into 3399-controlled-access-FRA-Data-Files-page-3

4ad6999

andrew-jameson reviewed Jan 21, 2025

View reviewed changes

tdrs-backend/tdpservice/users/migrations/0042_user_feature_flags.py Outdated Show resolved Hide resolved

Update tdrs-backend/tdpservice/users/migrations/0042_user_feature_fla…

093c734

…gs.py Co-authored-by: Andrew <[email protected]>

andrew-jameson approved these changes Jan 21, 2025

View reviewed changes

elipe17 reviewed Jan 22, 2025

View reviewed changes

tdrs-backend/tdpservice/users/test/test_models.py Outdated Show resolved Hide resolved

elipe17 reviewed Jan 22, 2025

View reviewed changes

improved test and some linting

0356b72

raftmsohani requested a review from elipe17 January 27, 2025 04:14

elipe17 reviewed Jan 27, 2025

View reviewed changes

elipe17 requested changes Jan 27, 2025

View reviewed changes

correct null field

b353c2f

raftmsohani requested a review from elipe17 January 28, 2025 15:17

Merge branch 'develop' into 3399-controlled-access-FRA-Data-Files-page-3

2bd0f18

jtimpe reviewed Jan 29, 2025

View reviewed changes

raftmsohani and others added 2 commits January 29, 2025 13:09

Merge branch 'develop' into 3399-controlled-access-FRA-Data-Files-page-3

a5795c5

Added FRA filter for user with FRA access

1e672fa

raftmsohani requested review from jtimpe and andrew-jameson January 29, 2025 20:15

raftmsohani commented Jan 29, 2025

View reviewed changes

changed filtering

2b0e4be

raftmsohani commented Jan 29, 2025

View reviewed changes

jtimpe approved these changes Jan 30, 2025

View reviewed changes

elipe17 approved these changes Jan 30, 2025

View reviewed changes

raftmsohani requested review from ADPennington and removed request for andrew-jameson January 30, 2025 16:16

raftmsohani added QASP Review and removed raft review This issue is ready for raft review labels Jan 30, 2025

Merge branch 'develop' into 3399-controlled-access-FRA-Data-Files-page-3

8edc8e0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

3399 controlled access fra data files page #3430

3399 controlled access fra data files page #3430

raftmsohani commented Jan 16, 2025 •

edited by elipe17

Loading

codecov bot commented Jan 21, 2025 •

edited

Loading

andrew-jameson commented Jan 21, 2025

elipe17 Jan 22, 2025

raftmsohani Jan 26, 2025

elipe17 Jan 22, 2025

raftmsohani Jan 26, 2025

elipe17 Jan 22, 2025

elipe17 left a comment

elipe17 Jan 27, 2025

raftmsohani Jan 28, 2025

raftmsohani Jan 28, 2025

elipe17 left a comment

elipe17 commented Jan 27, 2025

raftmsohani commented Jan 28, 2025

jtimpe left a comment

jtimpe Jan 29, 2025

raftmsohani Jan 29, 2025 •

edited

Loading

raftmsohani Jan 29, 2025

jtimpe left a comment

		@@ -37,6 +37,16 @@ class Media:

		actions = ['reparse']

		def get_queryset(self, request):

3399 controlled access fra data files page #3430

Are you sure you want to change the base?

3399 controlled access fra data files page #3430

Conversation

raftmsohani commented Jan 16, 2025 • edited by elipe17 Loading

Summary of Changes

How to Test

Deliverables

codecov bot commented Jan 21, 2025 • edited Loading

Codecov Report

andrew-jameson commented Jan 21, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elipe17 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elipe17 left a comment

Choose a reason for hiding this comment

elipe17 commented Jan 27, 2025

raftmsohani commented Jan 28, 2025

jtimpe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raftmsohani Jan 29, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtimpe left a comment

Choose a reason for hiding this comment

raftmsohani commented Jan 16, 2025 •

edited by elipe17

Loading

codecov bot commented Jan 21, 2025 •

edited

Loading

raftmsohani Jan 29, 2025 •

edited

Loading