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(Collector): Add support to have alternative import column names #37

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Hipska
Copy link
Collaborator

@Hipska Hipska commented Sep 1, 2023

This solves issues when attribute code does not match import column name or if an attribute has multiple columns.

Custom collectors can now override HeaderIsAllowed method to specify if the column should be allowed or not.

@Hipska Hipska force-pushed the feature/import_columns branch from f197e6f to 9f204ea Compare September 1, 2023 11:38
@piRGoif piRGoif added the bug Something isn't working label Sep 1, 2023
@Hipska
Copy link
Collaborator Author

Hipska commented Sep 8, 2023

(not really a bug, more like a feature IMHO)

@piRGoif if the corresponding PR Combodo/iTop#541 would not be accepted, we can reduce this PR to only the addition of HeaderIsAllowed with the fallback method:

	protected function HeaderIsAllowed($sHeader)
	{
		return array_key_exists($sHeader, $this->aFields);
	}

@Hipska Hipska changed the title Add support to have alternative import column names feat(Collector): Add support to have alternative import column names Dec 22, 2023
@Hipska
Copy link
Collaborator Author

Hipska commented Jan 15, 2024

Can I have any feedback please?

@piRGoif
Copy link
Contributor

piRGoif commented Jan 31, 2024

Hello,
Any PR submitted has to be handled by a Combodo employee first, who will ensure she/he understand everything and the code is up to our standards. Then the PR will be sent in reviews.
I'm not quite an expert on collectors so I won't handle this PR. I'll ask other Combodo devs that are involved in collectors...

@Hipska
Copy link
Collaborator Author

Hipska commented Jan 31, 2024

Okay, thanks. I was mentioning you specifically since you added the bug label, while this is actually a new feature enhancement.

@piRGoif
Copy link
Contributor

piRGoif commented Jan 31, 2024

I do some triage but not always very at ease on some subjects O:)
I'll change the label

@piRGoif piRGoif added enhancement New feature or request and removed bug Something isn't working labels Jan 31, 2024
@Hipska
Copy link
Collaborator Author

Hipska commented Apr 30, 2024

@Molkobain do you have some (personal?) feedback?

@Molkobain
Copy link
Contributor

Not yet unfortunately, as I'm very not familiar with the collectors, I don't know how to test this before / after without a detailled reproduction procedure and test cases.

I know the PR template is a bit boring, but it's helps me a lot understanding and setuping the PR to handle it. Otherwise it demands me to spend a few hours to dig into it which I don't have often. :/

Comment on lines 579 to 581
foreach ($aHeaders as $sHeader) {
if (($sHeader != 'primary_key') && !array_key_exists($sHeader, $this->aFields)) {
if (($sHeader != 'primary_key') && !$this->HeaderIsAllowed($sHeader)) {
if (!$this->AttributeIsOptional($sHeader)) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Especially this part is key to allow for more customisation of the data collectors..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Pending technical review
Development

Successfully merging this pull request may close these issues.

3 participants