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

Cover edge cases for validations #146

Merged
merged 2 commits into from
Jul 1, 2024

Conversation

ericaporter
Copy link
Contributor

@ericaporter ericaporter commented May 24, 2024

When testing the validation that hidden_pii fields do not overlap analytics_pii fields I noticed that the validation method would error if either the allowlist_pii file were in any of these scenarios:

  1. shared:
  2. shared: candidates:
  3. shared: candidates: []

rather than shared: {} which is explicitly empty. To avoid this I updated the following methods to:

def self.hidden_pii
         DfE::Analytics.hidden_pii || {}
  end

def self.allowlist_pii
      DfE::Analytics.allowlist_pii || {}
end

and added in next if fields.nil? || fields.empty? to the overlap validation method

@ericaporter ericaporter requested a review from asatwal May 24, 2024 09:13
@ericaporter ericaporter self-assigned this May 24, 2024
@ericaporter ericaporter force-pushed the cover-edge-cases-for-validations branch from 0090256 to 53ddf4b Compare May 24, 2024 09:14
Copy link
Collaborator

@asatwal asatwal left a comment

Choose a reason for hiding this comment

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

LGTM 👍 - Just one minor nitpick.

lib/dfe/analytics/fields.rb Outdated Show resolved Hide resolved
@ericaporter ericaporter marked this pull request as ready for review June 27, 2024 10:10
@ericaporter ericaporter merged commit ff894de into hidden-pii-updates Jul 1, 2024
@ericaporter ericaporter deleted the cover-edge-cases-for-validations branch July 1, 2024 14:02
ericaporter added a commit that referenced this pull request Jul 8, 2024
* Add DATA_hidden to events table creation SQL script
* Add hidden PII required permissions to GCP custom IAM role setup instructions
* Add policy tag setup instructions to GCP setup instructions
* Update role configuration to match latest BAT configuration
* Add hidden_pii.yml (#121)
* Allow hidden data to be sent separately (#128)
* Mask hidden_pii from logs
* Add validation for hidden pii fields appearing on both lists
* Change DATA_hidden to hidden_DATA in google_cloud_bigquery_setup.md
* Change DATA_hidden to hidden_DATA in create-events-table.sql
* Cover edge cases for validations (#146)
* Update log masking method (#150)
* Add reference to hidden_pii to cutom events (#153)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants