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

DIGIT Team Group + Kibana + Queries #2861

Merged
merged 17 commits into from
Apr 3, 2024
Merged

Conversation

elipe17
Copy link

@elipe17 elipe17 commented Feb 22, 2024

Summary of Changes

How to Test

  • Uncomment REACT_APP_DEV_KIBANA from the frontend .env file
  • Set BYPASS_KIBANA_AUTH=True in the backend .env
cd tdrs-frontend && docker-compose up --build
cd tdrs-backend && docker-compose up --build
  1. Request access via the frontend with non-superuser account
  2. Log in as superuser and set the non-superusers group as DIGIT Team check is_staff
  3. Log back in as non-superuser
  4. Verify DIGIT user can't submit datafiles, and can view only the aforementioned objects in the DAC
  5. Verify the DIGIT user can navigate to Kibana
  6. As a user who can submit data, load the DB up with some records
  7. Verify the example queries (potentially with modifications depending on what datafile you submit and as what STT) execute and produce the desired result.

OFA_to_Kibana_Queries_with_Export.md

Deliverables

More details on how deliverables herein are assessed included here.

Deliverable 1: Accepted Features

Checklist of ACs:

  • User group should be able to access all raw data files in S3 via: /admin/data_files/*
  • User group should be able to access the most recent version of parsed and validated data that has been accepted into the Elastisearch postgres db
  • User group can access Kibana
  • User group can execute converted OFA SQL queries

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

- Updated frontend to allow digit access to admin and kibana
- Update test
- Add fixture
@elipe17 elipe17 added backend dev raft review This issue is ready for raft review DAC Django Admin Console labels Feb 22, 2024
@elipe17 elipe17 self-assigned this Feb 22, 2024
Copy link

codecov bot commented Feb 22, 2024

Codecov Report

Attention: Patch coverage is 83.33333% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 93.47%. Comparing base (e120f9d) to head (edcc1e7).
Report is 1 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2861      +/-   ##
===========================================
- Coverage    93.50%   93.47%   -0.04%     
===========================================
  Files          268      269       +1     
  Lines         6190     6219      +29     
  Branches       528      530       +2     
===========================================
+ Hits          5788     5813      +25     
- Misses         309      313       +4     
  Partials        93       93              
Flag Coverage Δ
dev-backend 93.63% <83.33%> (-0.05%) ⬇️
dev-frontend 92.62% <ø> (ø)

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

Files Coverage Δ
tdrs-frontend/src/selectors/auth.js 97.22% <ø> (ø)
...ackend/tdpservice/users/api/authorization_check.py 84.44% <0.00%> (ø)
tdrs-backend/tdpservice/users/models.py 92.13% <66.66%> (-0.89%) ⬇️
...s/migrations/0040_users_digit_group_permissions.py 88.46% <88.46%> (ø)

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 509309a...edcc1e7. Read the comment docs.

Copy link

@raftmsohani raftmsohani left a comment

Choose a reason for hiding this comment

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

need to rename the migration file

@elipe17 elipe17 requested a review from raftmsohani February 26, 2024 21:50
Copy link

@raftmsohani raftmsohani left a comment

Choose a reason for hiding this comment

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

LGTM!

'OFA System Admin',
'ACF OCIO',
'OFA Admin',
'DIGIT Team',
Copy link

Choose a reason for hiding this comment

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

this enables the Admin tab on the frontend if the user has DIGIT Team as a role - this is a little weird when the user does not have superuser or staff privileges (though i think that problem pre-exists the digit team role specifically) - just want to clarify if digit team members are expected to have admin privileges

Copy link
Author

Choose a reason for hiding this comment

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

They are expected to have Admin privileges in the same vain as ACF OCIO. The digit team can only see a very small subset of the entire admin console. Specifically, datafiles, datafile summary, and parser errors. I can double check with Alex if she wants all three or strictly just datafiles.

Copy link
Author

Choose a reason for hiding this comment

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

Just re-read the requirements of the file, and the team needs to see the parsed data (duh haha) so I am going to add view permission for that also.

Copy link

@jtimpe jtimpe left a comment

Choose a reason for hiding this comment

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

minor suggestion, otherwise looking good!

@elipe17 elipe17 requested a review from atrimpe March 4, 2024 17:18
@elipe17 elipe17 removed the raft review This issue is ready for raft review label Mar 9, 2024
@ADPennington ADPennington added Deploy with CircleCI-qasp Deploy to https://tdp-frontend-qasp.app.cloud.gov through CircleCI and removed Deploy with CircleCI-qasp Deploy to https://tdp-frontend-qasp.app.cloud.gov through CircleCI labels Mar 25, 2024
@ADPennington ADPennington added the Deploy with CircleCI-qasp Deploy to https://tdp-frontend-qasp.app.cloud.gov through CircleCI label Mar 26, 2024
@ADPennington ADPennington added Deploy with CircleCI-qasp Deploy to https://tdp-frontend-qasp.app.cloud.gov through CircleCI and removed Deploy with CircleCI-qasp Deploy to https://tdp-frontend-qasp.app.cloud.gov through CircleCI labels Mar 26, 2024
@reitermb reitermb mentioned this pull request Mar 26, 2024
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.

@elipe17 below is a summary of testing results. test notes here

Summary

  • digit group successfully added
  • digit group does have access to kibana endpoint
  • the kibana dev tools console queries, as documented in the markdown, isnt working properly. I know that you mentioned that there is an issue with the fields. In the meantime, we did confirm during our sync that KQL queries from the dashboard worked as expected.
  • cloud.gov upgraded ES version to 7.10 today, and im curious if this will give us the CSV export capability we'd like to have, and if there is a limit on the number of records that would be returned. our queries to the legacy db, for example, can return upwards of 600K records.
    • related, how do we specify in the query that we only want the most recent version of records only? Is this the default?

@ADPennington ADPennington added Blocked Label for Pull Requests that are currently blocked by a dependency and removed Deploy with CircleCI-qasp Deploy to https://tdp-frontend-qasp.app.cloud.gov through CircleCI labels Mar 28, 2024
@elipe17 elipe17 requested a review from ADPennington April 2, 2024 16:05
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.

@elipe17 approving this since ACs are met. 🚀 In response to my testing notes, I'm including takeaways from our sync below.

#2908 can be used to track follow-on work to facilitate OFA's access to parsed data.


@elipe17 below is a summary of testing results. test notes here

Summary

  • digit group successfully added

✔️

  • digit group does have access to kibana endpoint

✔️

  • the kibana dev tools console queries, as documented in the markdown, isnt working properly. I know that you mentioned that there is an issue with the fields. In the meantime, we did confirm during our sync that KQL queries from the dashboard worked as expected.

⚠️ while KQL query results can be retrieved using the markdown syntax provided, browser dev tools has to be enabled to export response, and this currently requires higher level security clearance and agency-level approval.

  • cloud.gov upgraded ES version to 7.10 today, and im curious if this will give us the CSV export capability we'd like to have, and if there is a limit on the number of records that would be returned. our queries to the legacy db, for example, can return upwards of 600K records.

⚠️ max # of records that can be exported, regardless of AWS ES version is 10K, which is a fraction of the expected number of records OFA typically retrieves for a given section, record type, and fiscal period.
⚠️ exporting parsed data via CSV is not possible using Kibana, which only facilitates exporting dashboards and visualizations

  • related, how do we specify in the query that we only want the most recent version of records only? Is this the default?

⚠️ via KQL, need created_at value for the latest file and include it in the query. we should be able to make a dashboard that contains info about the latest datafile's records, if OFA would find this useful.

@ADPennington ADPennington added Ready to Merge and removed QASP Review Blocked Label for Pull Requests that are currently blocked by a dependency labels Apr 3, 2024
@andrew-jameson andrew-jameson merged commit 7a062d8 into develop Apr 3, 2024
14 checks passed
@andrew-jameson andrew-jameson deleted the 1441-digit-access branch April 3, 2024 17:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

As OFA tech lead, I need a new permissions group for OFA data analysts
6 participants