Skip to content

Add Fi* objects and change some others. - #13

Open
quint-pruijssers wants to merge 4 commits into
rmuit:masterfrom
kjsoftware:fix/ADEZZ-6265
Open

Add Fi* objects and change some others.#13
quint-pruijssers wants to merge 4 commits into
rmuit:masterfrom
kjsoftware:fix/ADEZZ-6265

Conversation

@quint-pruijssers

Copy link
Copy Markdown

No description provided.

@rmuit rmuit changed the title Fix/adezz 6265 Add Fi* objects and change some others. Sep 4, 2026
@rmuit

rmuit commented Sep 4, 2026

Copy link
Copy Markdown
Owner

Thank you for contributing your definitions! That's exactly what this repository needs.
And sorry for leaving this unanswered for so long.

However, in order to add this upstream, some clarification / changes are needed. Please note I have not had access to an AFAS instance for the past ~7 years, so I can't test things myself.

One thing about the FI* objects:

  • You have undoubtedly seen the existing object definitions in UpdateObject::getDefaultPropertyDefinitions().
  • The new classes do not add any extra logic.

So why not just add the FI* definitions into UpdateObject? (There's nothing wrong with adding them as new classes, but it makes the code inconsistent, and I fear things will get harder to find then.)

I might still have merged this PR if that was the only thing, but there are more significant things - which I will post as review comments near the code.

'type' => 'blob',
],
],
];

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I assume you've seen the KnSubject definitions in UpdateObject. These are now not used anymore, so at the very least they should be cleaned up. Or: the definitions should be changed in there. (I don't know what is better, in this KnSubject case.)

Please clarify (mag ook in het Nederlands als je wilt) why you have done it this way, because the biggest change I see is that you have effectively deleted a lot of field definitions.

I see you've added `'type' => 'blob' to FileStream. That's fine. Also adding extra aliases is fine.

But if you are adding 'required', please specify that this is actually required globally in AFAS, not just for your instance.

Note: I may not have good enough knowledge of AFAS and the definitions I provided for KnSubject may be non-general == not applicable for this upstream repo. But if so: please tell me that.

Also, re. the aliases:

  • they are currently all 'English based', only lowercase + underscores.
  • This PR adds a mix of English + Dutch, with upper + lowercase + spaces.

If you want to have aliases like these in your local system: great. There are ways to override the aliases. But filing this as a PR upstream... just makes thing inconsistent.

// Default of true is set dynamically in
// validateFields(), depending on presence of
// KnAddress objects.
],

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

First of all: this should be moved down to around line 638 so it's better understandable what we are talking about here.

Then: there is already a field with alias postal_address_is_address. We are not going to have two fields with the same alias committed to this repository.

Your change looks very logical. But, as you can probably tell from line ~638: 'PadAdr' did not work for me in KnOrganisation object; this was called 'PbAd' for me.

  • Did AFAS change this in the past ~9 years while I wasn't looking? (Did AFAS document this anywhere?) Or was this a weird thing on my side?
  • Does 'PbAd' still exist, or is that now gone? (If it's gone: we should remove the 'PbAd' definition.)

(If this + the VaTP additions were the only changes in this PR, I would not wait to add it... but the 'PbAd' alias needs to be removed at the same time.)

// TypeRekening
'VaTp' => [
'alias' => 'type_verkooprelatie',
],

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Just checking, want Rekening != Verkooprelatie in mijn hoofd:

  • Is "TypeRekening" een omschrijving in AFAS docs? (Ik heb ook letterlijke AFAS omschrijvingen in de code geplempt, dus dat is best, het is beter dan niks als documentatie
  • Maar was is het echt? Verwijst het naar een rekening/account of naar een verkooprelatie?
  • Heeft dit "type" veld slechts een aantal toegestane waarden? Zijn dat nummers, of IDs van een ander type AFAS object?

Comment thread composer.json
@@ -1,5 +1,5 @@
{
"name": "wyz/practicalafas",
"name": "kjsoftware/practicalafas",

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I understand you need your own branch to refer to in your own build system or whatever. And you're solving internal tickets in that branch.

But don't put this in a PR that you submit to upstream. Create another branch without this change.

(Otherwise I might still add selected code changes... but copypasting them manually and closing this PR.)

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.

3 participants