feat: add invoice creation #6

mrnagydavid · 2023-06-24T15:25:22Z

The parameters follow the same shape as described in the Szamlazz.hu API documentation.

    ExSzamlazzHu.create_invoice(%{
        beallitasok: %{
          szamlaagentkulcs: "your Szamlazz.hu agent key",
          eszamla: true,
          szamlaLetoltes: false,
          valaszVerzio: 1,
        },
        fejlec: %{
          teljesitesDatum: "2023-11-12",
          fizetesiHataridoDatum: "2023-11-12",
          fizmod: "Stripe",
          penznem: "EUR",
          szamlaNyelve: "en",
          megjegyzes: "",
          rendelesSzam: "Skynet-O129A22",
          dijbekero: false,
          fizetve: true
        },
        elado: %{},
        vevo: %{
          nev: "Sarah Connor",
          orszag: "USA",
          irsz: 32456,
          telepules: "Los Angeles",
          cim: "Engineering Drive",
          email: "[email protected]",
          sendEmail: true
        },
        tetelek: [
          %{
            megnevezes: "T-800 disassembly kit",
            mennyiseg: 2,
            mennyisegiEgyseg: "db",
            nettoEgysegar: 100,
            afakulcs: 27,
            nettoErtek: 200,
            afaErtek: 54,
            bruttoErtek: 254,
          }
        ]
      })

The result of the call is a struct, which - among other things - contains the original response from the Szamlazz.hu API.

      {:ok, %ExSzamlazzHu.Modules.CreateInvoice.Result{success: true}} = ExSzamlazzHu.create_invoice(params)

      %ExSzamlazzHu.Modules.CreateInvoice.Result{
        success: true,                # Indicates whether the request to the szamla.hu API was successful
        raw_response: nil,            # The raw response from the szamla.hu API
        szlahu_id: nil,               # The (internal) ID of the created invoice
        szlahu_szamlaszam: nil,       # The invoice number
        szlahu_nettovegosszeg: nil,   # The net amount of the created invoice
        szlahu_bruttovegosszeg: nil,  # The gross amount of the created invoice
        szlahu_kintlevoseg: nil,      # The amount not yet paid
        szlahu_vevoifiokurl: nil,     # The URL of the invoice
        path_to_pdf_invoice: nil,     # The path to the created invoice, if the PDF file was requested
        szlahu_error: nil,            # The error message, if any (and in Hungarian)
        szlahu_error_code: nil,       # The error code
        szlahu_down: false            # Indicates whether the Szamlazz.hu API is not available
      }

I added tests hitting the szamlazz.hu API, they are tagged with :external and this tag is exlcuded from mix test by default, but they run on the CI.

We are using Tesla, so that the actual http lib can be chosen by the user.
I configured the repo to use Hackney instead of the default httpc (for dev and test) when I saw that we receive {:error, :remotely_closed_by_peer} errors. That didn't solve it, and we now receive {:error, :closed} randomly. I added a workaround to attempt the calls twice in the tests. I have this issue in production apps too, so maybe it is a szamlazz.hu issue...

JustMikey · 2023-06-24T15:27:37Z

I would prefer to have 3 separate modules, one for invoice, one for customer and for items.

JustMikey · 2023-06-24T15:29:00Z

lib/modules/create_invoice/invoice_data.ex

+    :is_paid,
+    :customer,
+    :items
+  ]


I think it should at the top and we should also use enforce_keys:

module MyMosule do @enforce_keys [:foo, :bar] defstruct @enforce_keys end

I also considered the @enforce_keys.
But the error message they return is very ugly.

** (ArgumentError) the following keys must also be given when building struct Car: [:make] expanding struct: Car.__struct__/1

If we simply do not care, but make the request, the Szamlazz.hu API will return an error anyway, which we will format/pass-along anyway.

One reason I wanted to use Ecto was this, to be able to validate and provide nice error messages.

I found accidentally another package that does exactly this: https://hexdocs.pm/construct/readme.html
It looked really well, but the formatter was having a hard time with it.
And I realized it has only 50 stars, and maybe we should not include less-tested dependencies.

We are going to be building the structure inside the package, it won't be responsibility of the client, so the errors won't reach the client as well. So it's just an extra protection to make sure we don't miss any of the keys when initializing the structure.

Sorry, I don't get it.

What do you mean "it won't reach the client"? What will happen with such an error?

How I imagine it, the client will call something like Szamlazz.create_invoice(client, %{a: b, c: d}) and then we take those params, do some basic validation (all required keys are there and they have correct types) and then build the internal records, i.e. Invoice, Customer and etc, so by the time we have to build those structs, we already know that all keys are there, so we shouldn't get an error that some keys were not provided. And if we do, then we have a bug. Or how do you see it?

I am confused.

In #1 you wrote:

I imagine we want Ecto for the validations, but the question is whether it should be our responsibility to validate the params. I am not sure, but I think we should just provide documentation, i.e. what fields we expect, which of them are mandatory, and what types they must have. The rest should be validated on the level above. Whatcha think?

Above you wrote:

we take those params, do some basic validation, [...] then build the internal records

Let's clear this.

If we offer validation for the input params, did you imagine something with guards and manual checks?

Ecto is probably an overkill for this, tho quite possibly most client applications would be using ecto anyway.
Maybe as a "peer dependency"?

The Construct package I linked can validate nicely, I am just not sure about bringing it to the repo.

I am also content with not providing validation. Maybe just in the beginning.
Because Szamlazz API is documented. They will need to consult it even if they use this library.
The Szamlazz API will validate the XML anyway.
So, I can argue that this library should be a thin wrapper on top of the Szamlazz API. Just a messenger.

I tried checking ExAws code. My impression is that it uses AWS to validate the request, and raises in rare cases when the request makes no sense.

https://github.com/ex-aws/ex_aws/blob/v2.4.3/lib/ex_aws/operation/query.ex#L41

https://github.com/ex-aws/ex_aws/blob/v2.4.3/lib/ex_aws/operation/json.ex#L63

I don't think we should be doing any thorough validations, i.e. making sure that the provided values are actually something that their server expects. We just check that the required keys are provided and maybe? that they have correct types, everything else is not out responsibility, how I see it. And for that, we don't need Ecto or any other package. And then we should also be careful with extracting the errors from the response, if the client provided incorrect data and the server rejected it.

So something like that, I hope it clears things out.

JustMikey · 2023-06-24T15:32:27Z

lib/modules/create_invoice/xml_builder.ex

+      xsi:schemaLocation="http://www.szamlazz.hu/xmlszamla https://www.szamlazz.hu/szamla/docs/xsds/agent/xmlszamla.xsd"
+    >
+      <beallitasok>
+        <szamlaagentkulcs>#{invoice_data.agent_key}</szamlaagentkulcs>


I don't think this agent_key should come as part of the invoice. That is, if it's going to be used in other places.

I probably don't understand how you would do it.

Having it as a global configuration is something that is discouraged, as far as I understood.
The main cited reason is that if your library is used by multiple dependencies, then one will overwrite the configuration of the other.

In our case, I can imagine an application that wants to create invoices for multiple customers, where each customer has their own API_KEY.
I was considering a client-based approach, e.g:

client = ExSzamlazzHu.client(api_key: "secret") ExSzamlazzHu.create_invoice(client, %{})

But then I realized that the key can be part of the params.

What would be your approach and why?

Yeah, I definitely like that much more.

I didn't know about this issue with the global config, I will read about it.

Yes, but...

why?

Because this way we separate params that are specific to the operation from the global params that are required for each operation.

So basically:

Szamlazz.create_invoice(client, %{amount: 100})

seems better than mixing two domains:

Szamlazz.create_invoice(%{amount: 100, agent_key: "sadfsfd"})

And it's easier to reuse the config as well, you just build it once and then use for each operation.

I agree, it feels more professional to have something like

Szamlazz.create_invoice(client, %{amount: 100})

and it indeed seems better than mixing two domains.

But the Szamlazz API mixes the domains. Rather than authorizing yourself and receiving a token/client, you put your api key into the XML each time. With every request, the client needs to send the credentials, settings and the invoice parameters.

I think there is value in following the Szamlazz API as close as possible, and providing only a thin wrapper. Something that hides the fact that the communication is with XML payloads and the response is plain text.
So this library can take an object (structured similarly to the XML), and return proper error codes and error messages.

So, at least in the beginning, I would like to go with the less elegant approach.

closer to the logic of the API, so the Szamlazz documentation will be more meaningful for the users of this library

easier input param validation, because we expect params in the structure of the XML that we are going to build

Once we handle every request, it can be a good point to look for optimization possibilities.
I am for it, I just don't think it is the right time.
It can even be a major change, but let's do it once we have the internals working.

I didn't know about that -> response is plain text. That's fucked up.

Frankly, I don't know that much about their API, you had way more experience with it, so if you feel it should be closer to how they manage things, then it's ultimate your call. I am mostly a sidekick here.

JustMikey · 2023-06-24T15:43:25Z

lib/modules/create_invoice/invoice_data.ex

@@ -0,0 +1,42 @@
+defmodule ExSzamlazzHu.Modules.CreateInvoice.InvoiceData do


I don't like InvoiceData, the Data part doesn't provide any information.

Okay...
It is "something" that will be sent to the API so that an invoice is created from it.
It cannot be named Invoice because it is not an invoice.
It may be InvoiceParams, as they are the parameters we expected to receive.
And since this is in the scope of "CreateInvoice" maybe that prefix can be omitted: Params.
Or I still think it can be InvoiceData, because it carries data that should be presented on the invoice.

What else?

Ah I see, that's a good point.

What if we call it Header then, since that's what it looks like to be for. And then unnest the other structs and ask the client to provide three structs separately:

create_invoice(%{header: header, customer: customer, items: items})

JustMikey · 2023-06-24T15:44:54Z

Also the type definitions should show what values can be nil, coz I think not all the fields are always mandatory.

mrnagydavid · 2023-06-24T15:49:07Z

Also the type definitions should show what values can be nil, coz I think not all the fields are always mandatory.

Yes. I will read thru the XSD and update the types accordingly.
Right now, let's focus on the file/folder structure, and maybe some internals.

mrnagydavid · 2023-06-25T12:39:41Z

I pushed a commit where I added my idea for validation.

I did it differently than what you suggested: I first parse the params into the struct and then validate it.

params = %{settings: %{e_invoice: true, download_invoice: false, response_version: "3"}}                               

params 
|> ExSzamlazzHu.Modules.CreateInvoice.InvoiceData.parse() 
|> ExSzamlazzHu.Modules.CreateInvoice.InvoiceData.validate()

# --> {:error, %{settings: %{response_version: :invalid}}}

It is simple, it returns either :ok, or marks the problematic fields as :invalid.
It works on nested structs.
I think the validation is rather simple:

# invoice_data.ex
  def validate(struct) do
    %{
      settings: &Settings.validate/1
    }
    |> Validator.validate(struct)
  end

# settings.ex
  @spec validate(t()) :: boolean()
  def validate(struct) do
    %{
      user: &(is_nil(&1) or is_binary(&1)),
      password: &(is_nil(&1) or is_binary(&1)),
      agent_key: &(is_nil(&1) or is_binary(&1)),
      e_invoice: &is_boolean/1,
      download_invoice: &is_boolean/1,
      download_invoice_number_of_copies: &(is_nil(&1) or is_integer(&1)),
      response_version: &(is_nil(&1) or is_integer(&1)),
      aggregator: &(is_nil(&1) or is_binary(&1)),
      guardian: &(is_nil(&1) or is_boolean(&1)),
      article_identifier_invoice: &(is_nil(&1) or is_boolean(&1)),
      external_invoice_identifier: &(is_nil(&1) or is_binary(&1))
    }
    |> Validator.validate(struct)
  end

If you have a different idea, would you show a code snippet, please?

I think this is simple enough.
It provides help for required values and types too.
I don't want to write a validation library.

JustMikey · 2023-06-26T08:12:56Z

is_boolean

I would prefer to have something more well structured, for example what they have here -> https://pspdfkit.com/blog/2020/declarative-validation-with-elixir/

Maybe a bit simpler though.

JustMikey · 2023-07-03T07:08:14Z

I'd consider using https://github.com/thoughtbot/ex_machina/

JustMikey · 2023-07-03T07:10:56Z

test/lib/modules/create_invoice/mpl_test.exs

+                  weight: :invalid,
+                  extra_services: :invalid,
+                  value_statement: :invalid
+                }}


I don't like those sheets of data structures, maybe it's just me but it's not extremely readable.

params = %{..} assert {:error, error} = validate() assert error.barcode == :invalid ....

JustMikey · 2023-11-13T21:40:36Z

lib/ex_szamlazz_hu.ex

+  | Query receipt            | ❌ |
+  | Send receipt             | ❌ |
+  | Query taxpayers          | ❌ |
+  | Create supplier account  | ❌ |


That's cute

JustMikey · 2023-11-13T21:42:17Z

lib/modules/create_invoice.ex

@@ -0,0 +1,135 @@
+defmodule ExSzamlazzHu.Modules.CreateInvoice do


What is the Modules namespace for?

JustMikey · 2023-11-13T21:42:53Z

lib/modules/create_invoice.ex

+  defp save_temporary_file(xml) do
+    random_chars = for _ <- 1..5, into: "", do: <<Enum.random(?a..?z)>>
+    timestamp = DateTime.utc_now() |> DateTime.to_iso8601()
+    filename = "#{timestamp}_#{random_chars}.xml"


I have a feeling you will be reusing this logic, so probably makes sense to put it somewhere else

JustMikey · 2023-11-13T21:43:19Z

lib/modules/create_invoice.ex

+         result <- compile_result(success, data, response) do
+      {:ok, result}
+    else
+      {:error, :cannot_save_temporary_file} -> {:error, :cannot_save_temporary_file}


JustMikey · 2023-11-13T21:44:10Z

lib/modules/create_invoice.ex

+    header_map = Map.new(response.headers)
+
+    cond do
+      header_map["szlahu_down"] == "true" -> {:ok, false, %{szlahu_down: true}}


It will be reused in all the requests, I imagine.

JustMikey · 2023-11-13T21:45:08Z

lib/modules/create_invoice.ex

+        szlahu_kintlevoseg: header_map["szlahu_kintlevoseg"],
+        szlahu_vevoifiokurl: header_map["szlahu_vevoifiokurl"]
+      }
+      |> maybe_add_invoice_path_info(response, invoice_data)


I would split it into 2 variables:

params = %{..} maybe_add_invoice(params)

Would be prettier than a pipe, IMHO

JustMikey · 2023-11-13T21:47:13Z

lib/modules/create_invoice.ex

+
+  defp get_invoice_pdf_data(
+         %Tesla.Env{} = response,
+         %InvoiceData{beallitasok: %{szamlaLetoltes: true, valaszVerzio: 2}}


I know you and Muriel have this cult of hating comments, but it would be cool to have some note about the thing with those versions 🙄

JustMikey · 2023-11-13T21:49:23Z

lib/modules/create_invoice/customer.ex

+  @spec to_xml(t()) :: String.t()
+  def to_xml(%__MODULE__{} = module) do
+    tags = [
+      nev: &"<nev>#{&1}</nev>",


First time I see this format, believe me or not.

JustMikey · 2023-11-13T21:50:07Z

lib/modules/create_invoice/header.ex

+    :elonezetpdf
+  ]
+
+  @spec parse(map()) :: t() | nil


Probably should be

@spec parse(map() | nil) :: t() | nil

JustMikey · 2023-11-13T21:55:13Z

lib/utils/struct_to_xml.ex

+        nil -> nil
+        value -> fun.(value)
+      end
+    end)


You can make this pipe prettier 😄👍🏻

mrnagydavid requested a review from JustMikey June 24, 2023 15:25

JustMikey reviewed Jun 24, 2023

View reviewed changes

mrnagydavid force-pushed the feat/add-create-invoice branch 2 times, most recently from f82c882 to 9b29da8 Compare June 25, 2023 14:07

JustMikey reviewed Jul 3, 2023

View reviewed changes

mrnagydavid force-pushed the feat/add-create-invoice branch from 41fa548 to b2f7a42 Compare November 12, 2023 13:54

mrnagydavid added 12 commits November 12, 2023 18:06

chore: gitignore .elixir_ls

179883b

chore: show structure for discussion

902e85c

chore: separate structs

70d6eab

chore: add validation example

4de1c42

chore: add create-invoice types

599a707

test: add tests for waybills

f296d09

test: add tests for item and item_ledger

f987444

chore: remove validations

f01ee17

chore: rename e_invoice to is_e_invoice

222329f

chore: set default response_version to 1

6f907a3

chore: translate props to Hungarian

3eec725

feat: render xml

3fe6010

mrnagydavid force-pushed the feat/add-create-invoice branch from ba70aa3 to 3fe6010 Compare November 12, 2023 17:07

mrnagydavid added 3 commits November 12, 2023 19:54

feat: make request to szamlazz.hu

cc19da0

chore: add moduledoc tag

d47b085

chore: fix credo warnings

8146559

mrnagydavid added 8 commits November 13, 2023 15:54

feat: return with struct result

f084a4d

test: run external tests in the pipeline

1c127b7

chore: update docs

8ad599a

fix: fix formatting

ac80cea

chore: update test pipeline config

ee323a5

fix: fix test

ab2fff5

fix: fix comment formatting

1567e80

chore: replace httpc with hackney in tests

dffc86b

mrnagydavid changed the title ~~[DRAFT] feat: add invoice creation~~ feat: add invoice creation Nov 13, 2023

JustMikey reviewed Nov 13, 2023

View reviewed changes

lib/utils/struct_to_xml.ex Outdated

nil -> nil

value -> fun.(value)

end

end)

Copy link

Collaborator

JustMikey Nov 13, 2023

Choose a reason for hiding this comment

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

You can make this pipe prettier 😄👍🏻

mrnagydavid added 5 commits November 14, 2023 11:35

chore: remove Modules namespace

b508c12

chore: minor fixes and refactoring

00ed2d4

chore: add tool-versions and format according to elixir 1.14

b250416

fix: fix dialyzer issue

2d66b7a

test: add retry for szamlazz.hu test calls

9cfc2ff

mrnagydavid merged commit 893760f into develop Nov 14, 2023
1 check passed

mrnagydavid deleted the feat/add-create-invoice branch November 14, 2023 12:52

		@@ -0,0 +1,42 @@
		defmodule ExSzamlazzHu.Modules.CreateInvoice.InvoiceData do

		@@ -0,0 +1,135 @@
		defmodule ExSzamlazzHu.Modules.CreateInvoice do

feat: add invoice creation #6

feat: add invoice creation #6

Conversation

mrnagydavid commented Jun 24, 2023 • edited Loading

JustMikey commented Jun 24, 2023

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JustMikey commented Jun 24, 2023

mrnagydavid commented Jun 24, 2023

mrnagydavid commented Jun 25, 2023 • edited Loading

JustMikey commented Jun 26, 2023

JustMikey commented Jul 3, 2023

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JustMikey Nov 13, 2023 • 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

mrnagydavid commented Jun 24, 2023 •

edited

Loading

mrnagydavid commented Jun 25, 2023 •

edited

Loading

JustMikey Nov 13, 2023 •

edited

Loading