Power reviews integration #5258

Vagoasdf · 2024-09-04T19:12:55Z

Description Of Changes

Adding Support for Power Reviews Integrations

Auth Override Added because it was using a non-standard basic auth (doc said it was Oauth2, but it behaves like basic auth)

Request override Added to properly format the JSON Payload

Code Changes

Created new suite of test for Power Reviews
Adding request Override for Delete Action on Power Reviews
Adding Auth Override

Steps to Confirm

Power Reviews should appear as an option on Integrations
Said integration should work properly

Pre-Merge Checklist

All CI Pipelines Succeeded
Documentation:
- documentation complete, PR opened in fidesdocs
- documentation issue created in fidesdocs
Issue Requirements are Met
Relevant Follow-Up Issues Created
Update CHANGELOG.md

vercel · 2024-09-04T19:12:58Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Skipped Deployment

Name	Status	Preview	Comments	Updated (UTC)
fides-plus-nightly	⬜️ Ignored (Inspect)	Visit Preview		Sep 10, 2024 1:44pm

cypress · 2024-09-04T19:25:49Z

fides Run #9860

Run Properties: Passed #9860 • 30448bd657 ℹ️: Merge 9249476033dd817c096775719e53fd1ab001a8e3 into 84c8b1e2e2b728e20c631131d6c8...

Project	`fides`
Branch Review	`refs/pull/5258/merge`
Run status	`Passed #9860`
Run duration	`00m 37s`
Commit	`30448bd657 ℹ️: Merge 9249476033dd817c096775719e53fd1ab001a8e3 into 84c8b1e2e2b728e20c631131d6c8...`
Committer	`Bruno Gutierrez Rios`
View all properties for this run ↗︎

Test results
Failures	`0`
Flaky	`0`
Pending	`0`
Skipped	`0`
Passing	`4`
View all changes introduced in this branch ↗︎

removing private identities before uploading

We might want to double check the Authentication, as have no auth might hit us in the foot forward going. Also, we should not want the calls after privacy. Those are for testing. Saving those for reference right now

still to be fixed. Having some weird errors locally

galvana

Good work here @Vagoasdf. I just left a few comments

galvana · 2024-09-06T00:42:34Z

CHANGELOG.md

@@ -17,6 +17,8 @@ The types of changes are:

 ## [Unreleased](https://github.com/ethyca/fides/compare/2.44.0...main)

+- Adding erasure support for Power Reviews [#5258](https://github.com/ethyca/fides/pull/5258)


Make sure you put the entry under the correct header

Suggested change

- Adding erasure support for Power Reviews [#5258](https://github.com/ethyca/fides/pull/5258)

### Added

- Adding erasure support for Power Reviews [#5258](https://github.com/ethyca/fides/pull/5258)

galvana · 2024-09-06T00:44:12Z

data/saas/config/power_reviews_config.yml

+  name: Power Reviews
+  type: power_reviews


Let's match this to the way they spell their name PowerReviews (with no space)

Suggested change

name: Power Reviews

type: power_reviews

name: PowerReviews

type: powerreviews

Make sure you change the file names and any usages of the type (powerreviews) and name (PowerReviews)

I'll do this change, but im slightly worried that it's going to negatively improve readability, since its all lowercase, and we have this double "r" in the middle

galvana · 2024-09-06T00:45:45Z

data/saas/dataset/power_reviews_dataset.yml

+    collections:
+      - name: privacy
+        fields:
+          - name: request_id
+            data_categories: [system.operations]
+            fidesops_meta:
+              data_type: string
+          - name: status
+            data_categories: [system.operations]
+            fidesops_meta:
+              data_type: string


This can be empty since we don't have an access request

Suggested change

collections:

- name: privacy

fields:

- name: request_id

data_categories: [system.operations]

fidesops_meta:

data_type: string

- name: status

data_categories: [system.operations]

fidesops_meta:

data_type: string

collections: []

galvana · 2024-09-06T00:47:09Z

tests/ops/integration_tests/saas/test_power_reviews_task.py

+            erasure_policy=erasure_policy_string_rewrite,
+            identities={"email": power_reviews_erasure_identity_email},
+        )
+        # We set the email to 1 since its 1 request only(?)


That's correct, the number corresponds to how many records we deleted in this case it would be just 1

galvana · 2024-09-06T00:49:36Z

...s/api/service/saas_request/override_implementations/authentication_strategy_power_reviews.py

+    client_secret: str
+
+
+class PowerReviewsAuthenticationStrategy(AuthenticationStrategy):


Could we have used the oauth2_client_credentials authentication strategy instead?

Initially i thought we wouldn't be able, since its a cross between Oauth2 and Basic auth, but it looks like it can be done. Updating it

galvana

Thanks for making the changes! I called out a few last things but you can go ahead and merge once those are done

galvana · 2024-09-09T19:52:02Z

data/saas/config/powerreviews_config.yml

+        value: 3
+
+  endpoints:
+    - name: privacy


Let's rename this to user so it reflects the type of object being deleted

Suggested change

- name: privacy

- name: user

galvana · 2024-09-09T19:52:35Z

tests/fixtures/saas/powerreviews_fixtures.py

+        "merchant_id": pydash.get(saas_config, "powerreviews.merchant_id")
+        or secrets["merchant_id"],
+        "merchant_group_id": pydash.get(saas_config, "powerreviews.merchant_group_id")
+        or secrets["merchant_group_id"],


We can remove these if we're not using them in the integration

Suggested change

"merchant_id": pydash.get(saas_config, "powerreviews.merchant_id")

or secrets["merchant_id"],

"merchant_group_id": pydash.get(saas_config, "powerreviews.merchant_group_id")

or secrets["merchant_group_id"],

galvana · 2024-09-09T19:53:04Z

tests/ops/integration_tests/saas/test_powerreviews_task.py

+            erasure_policy=erasure_policy_string_rewrite,
+            identities={"email": powerreviews_erasure_identity_email},
+        )
+        assert erasure_results == {"powerreviews_instance:privacy": 1}


To match the name change above

Suggested change

assert erasure_results == {"powerreviews_instance:privacy": 1}

assert erasure_results == {"powerreviews_instance:user": 1}

Correctly setting the name. Removing unused keys on fixutres

cypress · 2024-09-10T15:18:37Z

fides Run #9862

Run Properties: Passed #9862 • 77dabdd420: Power reviews integration (#5258)

Project	`fides`
Branch Review	`main`
Run status	`Passed #9862`
Run duration	`00m 36s`
Commit	`77dabdd420: Power reviews integration (#5258)`
Committer	`Bruno Gutierrez Rios`
View all properties for this run ↗︎

Test results
Failures	`0`
Flaky	`0`
Pending	`0`
Skipped	`0`
Passing	`4`
View all changes introduced in this branch ↗︎

Vagoasdf requested a review from a team as a code owner September 4, 2024 19:12

Vagoasdf added 16 commits September 4, 2024 16:00

Base PowerReviews Data from n8n set up

f96d0cb

removing private identities before uploading

Setting up Config Changes

537cf18

We might want to double check the Authentication, as have no auth might hit us in the foot forward going. Also, we should not want the calls after privacy. Those are for testing. Saving those for reference right now

Changing name from PowerReviews to Power_reviews

49d040f

Setting Up base Authentication Strategy

bd1bff3

still to be fixed. Having some weird errors locally

Setting base test and Fixtures

aba6f11

Correctly setting up Authentication strategy

9942325

Adding base limit to avoid high volume query

5ffc4dc

Removing unused nodes from dataset

7829c13

Removing unused nodes from config

6e1ad59

Updating Endpoint for Delete

d3430e9

Base request override configuration

538f1c5

Setting up Integration Override

c370d5f

Correctly naming the property in testing

057126b

Removing Unused Config

f5f4872

Statick checks improved

086d95e

Removing Unused Imports, Re structuring request comments

6715ec5

Vagoasdf force-pushed the PowerReviews-Integration branch from d0c2a6f to 6715ec5 Compare September 4, 2024 20:00

Updating Changelog

298c63f

Vagoasdf requested a review from galvana September 4, 2024 20:15

galvana requested changes Sep 6, 2024

View reviewed changes

Vagoasdf added 6 commits September 6, 2024 11:30

Adding Correct header to Changelog

fb5f602

Updating names on files in favour of powerreviews

4e44088

removing unused Collections

0e54e8a

Updating Strategy to use standard one

a5da233

Removing Whitespace after header

3c44f34

Setting Connector Version to 0.1.0

a304171

Vagoasdf requested a review from galvana September 6, 2024 16:07

Merge branch 'main' into PowerReviews-Integration

9bfd685

vercel bot deployed to Preview September 9, 2024 13:21 View deployment

galvana approved these changes Sep 9, 2024

View reviewed changes

Vagoasdf added 2 commits September 9, 2024 17:42

Final Fix for Powerreviews

a9792e8

Correctly setting the name. Removing unused keys on fixutres

Fixing Statick checks

9249476

Vagoasdf merged commit 77dabdd into main Sep 10, 2024
37 checks passed

Vagoasdf deleted the PowerReviews-Integration branch September 10, 2024 15:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Power reviews integration #5258

Power reviews integration #5258

Vagoasdf commented Sep 4, 2024 •

edited

Loading

vercel bot commented Sep 4, 2024 •

edited

Loading

cypress bot commented Sep 4, 2024 •

edited

Loading

galvana left a comment

galvana Sep 6, 2024

galvana Sep 6, 2024

Vagoasdf Sep 6, 2024 •

edited

Loading

galvana Sep 6, 2024

galvana Sep 6, 2024

galvana Sep 6, 2024

Vagoasdf Sep 6, 2024

galvana left a comment

galvana Sep 9, 2024

galvana Sep 9, 2024

galvana Sep 9, 2024

cypress bot commented Sep 10, 2024

		@@ -17,6 +17,8 @@ The types of changes are:

		## [Unreleased](https://github.com/ethyca/fides/compare/2.44.0...main)

		- Adding erasure support for Power Reviews [#5258](https://github.com/ethyca/fides/pull/5258)

	- Adding erasure support for Power Reviews [#5258](https://github.com/ethyca/fides/pull/5258)
	### Added
	- Adding erasure support for Power Reviews [#5258](https://github.com/ethyca/fides/pull/5258)

		client_secret: str


		class PowerReviewsAuthenticationStrategy(AuthenticationStrategy):

	assert erasure_results == {"powerreviews_instance:privacy": 1}
	assert erasure_results == {"powerreviews_instance:user": 1}

Power reviews integration #5258

Power reviews integration #5258

Conversation

Vagoasdf commented Sep 4, 2024 • edited Loading

Description Of Changes

Code Changes

Steps to Confirm

Pre-Merge Checklist

vercel bot commented Sep 4, 2024 • edited Loading

cypress bot commented Sep 4, 2024 • edited Loading

galvana left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vagoasdf Sep 6, 2024 • edited Loading

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

galvana 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

cypress bot commented Sep 10, 2024

Vagoasdf commented Sep 4, 2024 •

edited

Loading

vercel bot commented Sep 4, 2024 •

edited

Loading

cypress bot commented Sep 4, 2024 •

edited

Loading

Vagoasdf Sep 6, 2024 •

edited

Loading