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

fix(backend): fix OP resource tests #2969

Merged
merged 20 commits into from
Oct 10, 2024

Conversation

njlie
Copy link
Contributor

@njlie njlie commented Sep 11, 2024

Changes proposed in this pull request

  • Adds an operatorApiSecret whose inclusion the headers will allow seeding to take place during localenv startup
  • Fixes the test environment such that the test app can start up with the Kratos changes
  • Fixes the tests for quotes, incoming payments (local & remote), outgoing payments on backend

Context

Fixes #2968.

This is part of a greater effort to bring the multi-tenant feature branch back up to par with the standards of the main branch, as a lot of tests and github actions are failing due to changes to the database that add foreign key constraints to many of the tables.

This PR fixes the tests for the Open Payments resources, as well as sets up some fixes for the test environment that gets set up by many of the tests.

Overall story is captured here: #2972

Checklist

  • Related issues linked using fixes #number
  • Tests added/updated
  • Make sure that all checks pass
  • Bruno collection updated (if necessary)
  • Documentation issue created with user-docs label (if necessary)
  • OpenAPI specs updated (if necessary)

@njlie njlie marked this pull request as draft September 11, 2024 21:10
@github-actions github-actions bot added type: tests Testing related pkg: backend Changes in the backend package. type: source Changes business logic pkg: mock-ase pkg: mock-account-service-lib labels Sep 11, 2024
@njlie njlie marked this pull request as ready for review September 11, 2024 21:29
Base automatically changed from nl/2915/tenant-management-ui to 2893-multi-tenant-rafiki September 16, 2024 16:25
An error occurred while trying to automatically change base from nl/2915/tenant-management-ui to 2893-multi-tenant-rafiki September 16, 2024 16:25
@njlie njlie force-pushed the nl-fix-tenant-feature-branch-tests branch from 365734f to a48dfb4 Compare September 16, 2024 17:42
@github-actions github-actions bot added the pkg: auth Changes in the GNAP auth package. label Sep 16, 2024
@njlie njlie force-pushed the nl-fix-tenant-feature-branch-tests branch from 02caf52 to 821c733 Compare October 3, 2024 21:09
@github-actions github-actions bot removed type: tests Testing related pkg: backend Changes in the backend package. pkg: mock-ase pkg: mock-account-service-lib labels Oct 3, 2024
@njlie njlie force-pushed the nl-fix-tenant-feature-branch-tests branch from 821c733 to 8c5667a Compare October 4, 2024 20:55
@github-actions github-actions bot added type: tests Testing related pkg: backend Changes in the backend package. pkg: frontend Changes in the frontend package. pkg: mock-ase pkg: mock-account-service-lib labels Oct 7, 2024
@@ -105,42 +105,6 @@ export function initIocContainer(
}
)

container.singleton(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For some reason, probably due to rebasing, this got duplicated a few times.

@@ -20,10 +20,6 @@ exports.up = function (knex) {
table.uuid('tenantId').notNullable()
table.foreign('tenantId').references('id').inTable('tenants')
})
.table('grants', function (table) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Grant's don't need tenantId - they're never queried on the backend by that filter, and the tenant it is for can be determined by the auth server URL path. Having the requirement explicitly in this table introduces some backwards compatibility issues.

@@ -14,45 +9,6 @@ export const mapTenantEndpointTypeToModelEndpointType = {
[EndpointType.WebhookBaseUrl]: TenantEndpointType.WebhookBaseUrl
}

export const getTenantEndpoints: TenantResolvers<ApolloContext>['endpoints'] =
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolver not being used anymore

Copy link
Contributor

Choose a reason for hiding this comment

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

looks like it leaves the service getTenantEndpointsPage method unused... remove?

@@ -1071,8 +1071,6 @@ input CreateQuoteInput {
receiver: String!
"Unique key to ensure duplicate or retried requests are processed only once. See [idempotence](https://en.wikipedia.org/wiki/Idempotence)"
idempotencyKey: String
"ID of the tenant"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

tenantId got removed from the quote resolver input for backwards compatibility issues and it can be retrieved from the wallet address anyways

Copy link
Contributor

@BlairCurrey BlairCurrey Oct 10, 2024

Choose a reason for hiding this comment

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

it can be retrieved from the wallet address anyways

I think perhaps this is the better pattern. It's simpler and I don't think we need to verify against the requestor's expectation for what the tenantId should be (ie, the tenantId being removed here).

I remember you mentioning that before and not having much of an opinion but I came across it myself when dealing with checking access for the createQuote resolver and felt like passing it in wasn't really providing any value, and it does make the gql input less complicated. Since before it was required for everyone but really only needed when an operator is making it on behalf of a tenant.

@@ -21,10 +21,6 @@ import {
import { v4 as uuid } from 'uuid'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The modifications here are all removing the tenantId parameter from grant creation calls

@njlie njlie requested a review from BlairCurrey October 7, 2024 20:45
@njlie njlie force-pushed the nl-fix-tenant-feature-branch-tests branch from 52767da to 3e84958 Compare October 10, 2024 17:04
@njlie njlie merged commit 7bcb13a into 2893-multi-tenant-rafiki Oct 10, 2024
16 of 26 checks passed
@njlie njlie deleted the nl-fix-tenant-feature-branch-tests branch October 10, 2024 17:34
@njlie njlie linked an issue Oct 11, 2024 that may be closed by this pull request
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: auth Changes in the GNAP auth package. pkg: backend Changes in the backend package. pkg: frontend Changes in the frontend package. pkg: mock-account-service-lib pkg: mock-ase type: source Changes business logic type: tests Testing related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Multi-tenant Rafiki] Update all resources to have a tenantId foreign key
4 participants