MegaChriz / afasprofit

An API to connect to Afas Profit written in PHP.
GNU General Public License v3.0
11 stars 7 forks source link

Request to UpdateConnector FiEntries endpoint fails #6

Closed 89peter closed 1 year ago

89peter commented 2 years ago

When I try to insert an entity using the FiEntries endpoint, it fails. It seems to me that there's a bug in the EntityValidator. But I'm not sure, though.

Eventually, the EntityValidator (the validateRecursively() function) is responsible for throwing an exception: Unknown type 'FiEntries'. I think that has everything to do with the entity that AFAS expects: an 'FiEntryPar' entity. This is probably an inconsistency in AFAS.

Could you verify if this is indeed true?

MegaChriz commented 2 years ago

Yes, the data to send via an update connector is validated against the XML schemes by default.

There is also a way to override the schemes with your own. I think it can be done using the code below, but I have not checked if this is everything you need (I copied this from my own application):

// Reset schema manager, use alternative XML schemes for validation.
$environment = 'A';
$manager = Afas::service('afas.xsd_schema.manager');
$manager->__construct();
$manager->addPath(dirname(__DIR__) . '/resources/XMLSchema/' . $environment);

I have no experience with the FiEntries connector. I do wonder if you would get a different XML scheme if you would generate them yourselves. I've added an example called 'data.php' to the examples folder that I have used to generate/update the XML schemes.

Can you try the following:

  1. Open examples/data.php in code editor.
  2. Change the login details for your Profit environment.
  3. Execute examples/data.php.

And then check if FiEntries is different?

MegaChriz commented 2 years ago

Can you also tell if the issue with FiEntries is introduced by the schema update? Or was it an issue already before? If it is introduced by the schema update, it would make sense to create a new release without the schema update. If it was already an issue, the schema update could be included with the release.

89peter commented 2 years ago

Hi,

I will look into this next week.

The issue was not introduced since the schema update, I already tested that. I got the exact same error after I required V3.1.

Outlook voor Androidhttps://aka.ms/AAb9ysg downloaden


From: Youri @.> Sent: Friday, September 23, 2022 5:01:20 PM To: MegaChriz/afasprofit @.> Cc: 89peter @.>; Author @.> Subject: Re: [MegaChriz/afasprofit] Request to UpdateConnector FiEntries endpoint fails (Issue #6)

Can you also tell if the issue with FiEntries is introduced by the schema update? Or was it an issue already before? If it is introduced by the schema update, it would make sense to create a new release without the schema update. If it was already an issue, the schema update could be included with the release.

— Reply to this email directly, view it on GitHubhttps://github.com/MegaChriz/afasprofit/issues/6#issuecomment-1256329659, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AEJIKE6AGENDMJP7CATZ43DV7XA4BANCNFSM6AAAAAAQTY3KXE. You are receiving this because you authored the thread.Message ID: @.***>

MegaChriz commented 2 years ago

I see that the FiEntries XSD file starts with a FiEntryPar element instead of a FiEntries element.

Does it work when you try to insert a FiEntryPar instead or does Profit not accept that?

The result of the following code does equal with the first XML example found on https://help.afas.nl/help/nl/se/App_Conect_UpdDsc_070.htm

$query = $server->insert('FiEntryPar', [
  'Year' => '2022',
  'Peri' => 1,
  'UnId' => 1,
  'JoCo' => 10,
  'FiEntries' => [
    [
      'VaAs' => 3,
      'AcNr' => 60014,
      'EnDa' => '2013-01-01',
      'BpDa' => '2013-01-01',
      'InId' => 'FACTUUR BTW1',
      'AmDe' => 0,
      'AmCr' => 12100,
      'DaEx' => '2013-02-01',
    ],
    [
      'VaAs' => 1,
      'AcNr' => 4210,
      'EnDa' => '2013-01-01',
      'BpDa' => '2013-01-01',
      'InId' => 'FACTUUR BTW1',
      'AmDe' => 10000,
      'AmCr' => 0,
      'VaId' => 5,
    ],
    [
      'VaAs' => 1,
      'AcNr' => 1500,
      'EnDa' => '2013-01-01',
      'BpDa' => '2013-01-01',
      'InId' => 'FACTUUR BTW1',
      'AmDe' => 2100,
      'AmCr' => 0,
      'VaId' => 5,
    ],
  ],
]);

$compiled = $query->getEntityContainer()->compile();

header('Content-Type: text/xml');
die($compiled);
MegaChriz commented 2 years ago

If it doesn't work, maybe we could fix it by adding an additional parameter called $entity_type_id to the following methods:

And if $entity_type_id has a value, pass that instead of $connector_id to EntityContainer in UpdateBase::__construct().

The contents of that method is now:

/**
   * Constructs a new UpdateBase object.
   *
   * @param \Afas\Core\ServerInterface $server
   *   The server to send data to.
   * @param string $connector_id
   *   The name of the UpdateConnector.
   * @param array $data
   *   The data to update.
   * @param array $attribute_keys
   *   (optional) The keys belonging to attributes.
   */
  public function __construct(ServerInterface $server, $connector_id, array &$data, array $attribute_keys = []) {
    parent::__construct($server);
    $this->connectorId = $connector_id;
    $this->entityContainer = new EntityContainer($connector_id);

    if (!empty($attribute_keys)) {
      if (ArrayHelper::isAssociative($data)) {
        $this->convertAttributes($data, $attribute_keys);
      }
      else {
        foreach ($data as &$subdata) {
          $this->convertAttributes($subdata, $attribute_keys);
        }
      }
    }
  }

Not tested if this would work, but it is my first guess on how the issue could be fixed.

89peter commented 1 year ago

Hi,

I see that the FiEntries XSD file starts with a FiEntryPar element instead of a FiEntries element. Does it work when you try to insert a FiEntryPar instead or does Profit not accept that?

Profit does indeed not accept that, it really expects an FiEntryPar entity.

So I've tried to make the changes you suggested and then changed our implementation so that the $entity_type_id is passed along with the FiEntries call. And it works, so that's great.

It might seem straightforward, but please note that not only UpdateBase::__construct() needs the extra parameter, but also the Insert::__construct() and Update::__construct() functions, and those objects need to be constructed properly in the Server::insert() and Server::update() functions.

MegaChriz commented 1 year ago

@89peter Do you want to share your code via a merge request? It may not be a lot of work to update the code myself, but if you've it ready, then that could save me time.

89peter commented 1 year ago

Sure, no problem. I've never worked with Github before, only with Gitlab. But it seems that I have to be added as a contributor to create a new branch, and then a pull request, right? Could you do that for me?

MegaChriz commented 1 year ago

I don't work with Github a lot, what I did in the past for other projects was:

89peter commented 1 year ago

Alright.

We create the branch from the PHP8.1 branch, right?

MegaChriz commented 1 year ago

I added you as a collaborator on the repo. That could mean you now have access to all branches, not sure yet how I can limit that to just the one linked with this issue: https://github.com/MegaChriz/afasprofit/tree/6-fientries-update-connector

MegaChriz commented 1 year ago

We create the branch from the PHP8.1 branch, right?

The branch is 6-fientries-update-connector.

89peter commented 1 year ago

I commited the changes, and I wrote some tests in ServerTest.php for the entity type. Please be kind, it's been a while since I wrote tests.

I've directed the pull request to the PHP8.1 support branch, is that okay?

MegaChriz commented 1 year ago

Thanks for writing tests too! 😀 I hadn't thought yet about that. These are indeed needed too.

Review:

The tests look okay except for that single test failure.

MegaChriz commented 1 year ago

It would be better if the pull request is directed to 3.x, php8.1 support branch has already been merged. I left it open for now to remind myself to create a new release.

89peter commented 1 year ago

Thanks for reviewing. Those were some sloppy mistakes.

I pushed 2 new commits in which the issues are fixed for both the review and the test. The CI jobs have passed, now.

MegaChriz commented 1 year ago

Thanks for the update! I've made some small changes to code comments and renamed the property $entity_type_id in UpdateBase to $entityTypeId.

In the comments, I've changed 'The entity type to ...' to 'The type of entity to ...' because you are not modifying an entity type, but an entity. It's a bit nitpicking, but as a perfectionist I like things to be correct. 🙂

89peter commented 1 year ago

I totally agree :ok_hand:

So the next steps are:

  1. merging the code into the 3.x branch;
  2. and then creating a release;

Will the release be a release candidate, as you said before in the other open issue? Or will it become v3.2?

MegaChriz commented 1 year ago

I've been running the latest code for more than a week now on a live site, so I think it's safe to go straight to the 3.2 release. I will merge the code now 🙂. For creating the release, I first want to write the release notes, so I'll see if I can get to that today.

MegaChriz commented 1 year ago

Changes are merged! The commit message ended up a bit differently than I what I expected it to be: it starts with "6 fientries update connector" instead of "Added feature to have the type of entity to create, update or delete be different from the connector type.".

89peter commented 1 year ago

It's the convention how Github creates names of branches from an issue. I'd say it's clear enough for most developers that it's about this issue (#6).

Thank you very much for acting so quickly!