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.

Sign up for GitHub

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

Jump to bottom

feat: add radom checkout session integration for new orders #2627

Merged
merged 12 commits into from
Sep 11, 2024
Merged

feat: add radom checkout session integration for new orders #2627

merged 12 commits into from
Sep 11, 2024

Conversation

clD11
Copy link
Contributor

@clD11 clD11 commented Aug 2, 2024 •
edited
Loading

Summary

This PR adds Radom checkout session integration for new orders.

Security Review: https://github.com/brave/reviews/issues/1719

resolves #2633

Type of Change

  • Product feature
  • Bug fix
  • Performance improvement
  • Refactor
  • Other

Tested Environments

  • Development
  • Staging
  • Production

Before Requesting Review

  • Does your code build cleanly without any errors or warnings?
  • Have you used auto closing keywords?
  • Have you added tests for new functionality?
  • Have validated query efficiency for new database queries?
  • Have documented new functionality in README or in comments?
  • Have you squashed all intermediate commits?
  • Is there a clear title that explains what the PR does?
  • Have you used intuitive function, variable and other naming?
  • Have you requested security and/or privacy review if needed
  • Have you performed a self review of this PR?

Manual Test Plan

@clD11 clD11 self-assigned this Aug 2, 2024
@clD11 clD11 changed the title Add Radom checkout session integration for new orders feat: add radom checkout session integration for new orders Aug 2, 2024
@clD11 clD11 force-pushed the pwb-01 branch 2 times, most recently from 1bf2b0d to 19b64fc Compare August 6, 2024 12:19
kdenhartog
kdenhartog approved these changes Aug 7, 2024
View reviewed changes
Copy link
Member

@kdenhartog kdenhartog left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Confirmed this is disabled by default at the moment. This LGTM from a privacy perspective.

bat-go/services/grant/cmd/grant.go

Line 146 in 9943700

).Bind("radom-enabled").Env("RADOM_ENABLED")

https://github.com/brave-intl/grant-ops/blob/master/ecs-vars-prod.env#L59

https://github.com/brave-intl/bat-go/pull/2627/files#diff-225dad392327a49c7aa0c5287edf78e66ae94759a42c08d4207299c0e0d6e895R235

@clD11 clD11 marked this pull request as ready for review August 13, 2024 13:55
@clD11 clD11 requested a review from pavelbrm August 13, 2024 13:56
pavelbrm
pavelbrm reviewed Aug 14, 2024
View reviewed changes
Copy link
Contributor

@pavelbrm pavelbrm left a comment

Choose a reason for hiding this comment

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

Overall looking good. Not ready to approve, and would like to see most of the comments addressed before this gets merged.

libs/clients/radom/radom.go Show resolved Hide resolved
services/skus/controllers.go Show resolved Hide resolved
services/skus/controllers_test.go Outdated Show resolved Hide resolved
services/skus/controllers_test.go Show resolved Hide resolved
services/skus/model/model.go Show resolved Hide resolved
services/skus/service.go Outdated Show resolved Hide resolved
services/skus/service_test.go Show resolved Hide resolved
services/skus/service.go Show resolved Hide resolved
services/skus/service_nonint_test.go Outdated Show resolved Hide resolved
services/skus/service_nonint_test.go Show resolved Hide resolved
pavelbrm
pavelbrm reviewed Aug 14, 2024
View reviewed changes
Copy link
Contributor

@pavelbrm pavelbrm left a comment

Choose a reason for hiding this comment

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

Also, the branch has not been updated since the last merge to master, and is currently in conflict. This needs to be resolved too.

pavelbrm
pavelbrm approved these changes Aug 15, 2024
View reviewed changes
Copy link
Contributor

@pavelbrm pavelbrm left a comment

Choose a reason for hiding this comment

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

Good work, approved! 👍

@github-actions GitHub Actions
Copy link

[puLL-Merge] - brave-intl/bat-go@2627

Description

This PR removes the Radom client implementation from the libs/clients/radom package and refactors the Radom-related functionality within the services/skus package. The changes aim to simplify the Radom integration and improve the overall structure of the codebase.

Changes

Changes

  1. libs/clients/radom/:

    • Removed the entire package, including instrumented.go, mock.go, and radom.go.

  2. libs/context/keys.go:

    • Removed Radom-related context keys.

  3. services/grant/cmd/grant.go:

    • Removed Radom-related context value assignments.

  4. services/skus/controllers.go:

    • Renamed HandleRadomWebhook to handleRadomWebhook.
    • Simplified the handleRadomWebhook function to return a "Not Implemented" response.

  5. services/skus/controllers_test.go:

    • Added a test case for Radom payable order creation.

  6. services/skus/model/model.go:

    • Added RadomMetadata and ItemRadomMetadata structs.
    • Removed Radom-specific methods from the Order struct.
    • Added RadomProductID method to OrderItem.

  7. services/skus/radom/radom.go:

    • Added a new package for Radom-related functionality.
    • Implemented a simplified Client struct for Radom API interactions.

  8. services/skus/service.go:

    • Refactored Radom-related code to use the new radom package.
    • Updated the Service struct to include radomClient and radomGateway.
    • Implemented createRadomSession method.

  9. services/skus/service_nonint_test.go and service_test.go:

    • Added tests for the new Radom-related functionality.

Possible Issues

  1. The simplified handleRadomWebhook function returns a "Not Implemented" response, which may break existing functionality if Radom webhooks were being used.

  2. The removal of Radom-related context keys and values may affect other parts of the application that were relying on these values.

Security Hotspots

No significant security hotspots were identified in this change. However, it's important to ensure that the new Radom client implementation in services/skus/radom/radom.go properly handles authentication and sensitive data.

@clD11
feat: handle radom webhook events (#2640)
72ecf54 
* feat: add radom checkout session integration for new orders

* fix: rectify skus model tests after rebase

* refactor: address comments

* fix imports
* update gateway to pointer
* update radom checkout session request to pointer
* rename order items to line items function
* add radom product id getter on metadata

* feat: add radom checkout session integration for new orders

* feat: handle radom webhook events

* test: remove duplicate test and fix pointer after rebase

* feat: set radom webhook verification key

* refactor: use pointers for radom events

* refactor: fix import order skus controller

* refactor: simplify radom event processing checks

* style: fix parse apostrophe

* refactor: use pointer for radom subscription response

* refactor: remove call to error when calling err on logging

* refactor: use index directly on radom mdata

* refactor: make radom action error naming less generic

* refactor: use pointer for service in test

* refactor: sort imports in radom.go

* refactor: update radom empty payments error message

* style: return from func call opposed to explicit return nil

* refactor: use string in radom get subscription

* refactor: add on conflict do nothing to order history insert statement

* refactor: rename radom event to radom notification

* test: remove duplicate test after rebase
@clD11 clD11 merged commit e3b825a into master Sep 11, 2024
12 checks passed
@clD11 clD11 deleted the pwb-01 branch September 11, 2024 18:59
@pavelbrm pavelbrm mentioned this pull request Sep 12, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kdenhartog kdenhartog kdenhartog approved these changes

@pavelbrm pavelbrm pavelbrm approved these changes

Assignees

@clD11 clD11

Labels
puLL-Merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement Radom checkout for new orders
3 participants
@clD11 @pavelbrm @kdenhartog