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

Patches For 1789 #1892

Merged
merged 11 commits into from
Jul 17, 2023
Merged

Patches For 1789 #1892

merged 11 commits into from
Jul 17, 2023

Conversation

pavelbrm
Copy link
Contributor

@pavelbrm pavelbrm commented Jul 14, 2023

This PR proposes a set of patches for #1789, and is a result of my review. Due to the high amount of suggestions, I decided to help implement them instead of risking to overwhelm the author.

Here's a [non-exhaustive] list of changes, in no particular order:

  • Synced the branch with master.
  • Ran go mod tidy for the modules in the monorepo to clean up some of the unnecessary records the original PR was going to bring in to the code base.
  • Updated the implementation of the Radom client:
    • We don't need to rely on mocking libraries for mocks. See the example mock implementation and in model/model_tests.go a set of tests using it.
    • We don't need to rely on code generation for wrapping with metrics. The generated code is clunky (uses global vars and a value receiver on the type which is held inside an interface variable) and can be inefficient, plus it adds unnecessary layers of abstractions and forces us go against the Go's idea of interfaces (inverts the dependency path – we have to declare the client interface upfront instead of declaring it where it is needed and used). We can keep the old code as it is, but for new code we can for now tolerate some minor manual labour. I need to find a way to improve the code generated wrappers to accomplish a similar result to what is currently done by hand.
    • The configuration for the client must be supplied from the outside. Ideally, all externally-declared configuration should be obtained as close to main as possible, and explicitly passed down. Hiding environment variables deep in the code is not a good idea.
    • Service now obtains all the configuration necessary for the creation of a client, and passes it to the constructor.
  • Updated minor details for the implementation of the Radom client data structures.
    • Specifically, when we are declaring custom types based on the [sometimes called, wrongly] reference types in Go (slices, maps, channels and functions), we normally declare methods on value receivers. It's fine to do so on a pointer receiver too, but in the case of these collection types those better be reserved for operations altering their internal structure (like appending, for example).
  • The new code added to the Postgres datastore in SKUs service has been moved to its proper home, the Order repository. Also, added tests for that new code.
  • Removed the unnecessary const referring to the Radom payment method as it can be used directly.
  • One of the important things, accepted chains and tokens, I think they belong in the client configuration, not at the level of orders. Plus, I don't like keeping them as global vars. Proposing to make them part of the client state, and make the client responsible for setting them in a request. Order should not be aware of those, unless I am missing something. This PR does that.
  • Similarly, I think that the seller address is part of the client state, and should be moved there too. It does not change for different orders, does it? For now I kept it, but only because wanted to clarify.
  • Updated the implementation for CreateRadomCheckoutSession.
    • The expiration time became an argument (so that we can later test it).
    • Instead of the ad-hoc errors, declared sentinel errors which can be tested against.
    • And wrote tests for that method, which at least proves that it's logic is solid.
    • That also showcases the use of the mocked client – nice, easy and declarative, as well as nice and declarative style for tests.
  • Updated CreateOrderFromRequest.
    • The previous suggested implementation might have resulted in paying an order with two systems (if we messed up with metadata; unlikely, but the way the code was written – possible). So now it's one or the other.

Not mentioning other really minor things.

This PR is currently aiming at the #1789 's branch. But it can as well be aimed at master. I run tests, they are passing (mind those green ticks in the list of commits 😉 ).

NOTE: I did not do anything re: other comments in the original PR provided by the other reviewer as I am lacking context for that.

Hope it makes sense.

@pavelbrm pavelbrm changed the base branch from pay-with-bat-skus to master July 14, 2023 08:19
@pavelbrm pavelbrm marked this pull request as ready for review July 14, 2023 08:30
@pavelbrm pavelbrm changed the base branch from master to pay-with-bat-skus July 14, 2023 09:08
@pavelbrm pavelbrm self-assigned this Jul 14, 2023
@pavelbrm pavelbrm mentioned this pull request Jul 14, 2023
18 tasks
@husobee husobee merged commit aef7a33 into pay-with-bat-skus Jul 17, 2023
@husobee husobee deleted the patches-1789 branch July 17, 2023 13:18
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