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

feat(frontend): migrate transactions to resource architecture #2493

Merged

Conversation

jorgeepc
Copy link
Contributor

@jorgeepc jorgeepc commented May 5, 2023

This PR updates the FE to work with the new transactions as resources BE changes

Changes

  • Fixes minor be bugs
  • Implements new types for the FE
  • Removes the triggerSettings object for triggers

Fixes

Checklist

  • tested locally
  • added new dependencies
  • updated the docs
  • added a test

@jorgeepc jorgeepc force-pushed the 2358-transactions-to-resource-fe branch from 434651c to cf183f0 Compare May 5, 2023 19:23
@jorgeepc jorgeepc changed the base branch from main to feat/transactions-as-resources May 5, 2023 22:34
@jorgeepc jorgeepc changed the title fix(frontend): migrate transactions to resource architecture feat(frontend): migrate transactions to resource architecture May 5, 2023
@schoren schoren force-pushed the feat/transactions-as-resources branch from f810cf8 to 0d2c098 Compare May 18, 2023 14:58
@xoscar xoscar self-assigned this May 19, 2023
$ref: "./grpc.yaml#/components/schemas/GRPCRequest"
traceid:
$ref: "./traceid.yaml#/components/schemas/TRACEIDRequest"
http:
Copy link
Collaborator

Choose a reason for hiding this comment

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

There's a mismatch between the resource version and the regular version, I removed the triggerSettings to have them both work the same

Http: m.HTTPRequest(in.HTTP),
Grpc: m.GRPCRequest(in.GRPC),
Traceid: m.TRACEIDRequest(in.TraceID),
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

Removing trigger settings

@@ -66,7 +66,7 @@ func (t Transaction) NewRun() TransactionRun {
return TransactionRun{
TransactionID: t.ID,
TransactionVersion: t.GetVersion(),
CreatedAt: time.Now(),
CreatedAt: time.Now().UTC(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

This was a bug, we need to always use UTC from the server

return Transaction.FromRawTransaction(rawTransaction);
}

Transaction.FromRawTransaction = ({
Copy link
Collaborator

Choose a reason for hiding this comment

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

Currently, we have to support two versions of the transactions API object:

  • resource (from the resource endpoints)
  • regular raw version (from the result list home page)

import TransactionService from 'services/Transaction.service';
import {TTestApiEndpointBuilder} from 'types/Test.types';
import {TDraftTransaction} from 'types/Transaction.types';

const defaultHeaders = {'content-type': 'application/json', 'X-Tracetest-Augmented': 'true'};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Including the augmented header

Copy link
Collaborator

Choose a reason for hiding this comment

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

not sure how this works, but just to be safe: for requesting the yaml definition, you need to NOT pass the header.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yep we have that covered 😄

Copy link
Collaborator

@schoren schoren left a comment

Choose a reason for hiding this comment

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

Looks good! my only question is about requesting the YAML for transactions, where you need to not pass the augmented header, and I'm not sure if it's being passed or not. other than that looks great!

@xoscar xoscar merged commit 9813abb into feat/transactions-as-resources May 19, 2023
21 of 30 checks passed
@xoscar xoscar deleted the 2358-transactions-to-resource-fe branch May 19, 2023 14:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants