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

FORMS-674: Search #24

Merged
merged 37 commits into from
May 2, 2023
Merged

FORMS-674: Search #24

merged 37 commits into from
May 2, 2023

Conversation

rimi-itk
Copy link
Contributor

@rimi-itk rimi-itk commented Mar 6, 2023

https://jira.itkdev.dk/browse/FORMS-674

  • FORMS-674: Removed use of deprecated function utf8_encode
  • FORMS-674: Cleaned up and added search command

@rimi-itk rimi-itk force-pushed the feature/FORMS-674-search branch 2 times, most recently from cc6ef2f to e1b9c86 Compare March 14, 2023 13:43
@rimi-itk rimi-itk marked this pull request as ready for review March 21, 2023 12:59
@rimi-itk rimi-itk requested a review from jekuaitk March 21, 2023 12:59
## Example: Find Organisation by CVR number

```php

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add a working example here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes … when we have a working example …

README.md Outdated Show resolved Hide resolved
$model->brugertype = $egenskab->getBrugerTypeTekst();
}
foreach ($registrering->getRelationListe()->getAdresser() as $adresse) {
// TODO
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there still more todo here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know yet. I don't know if the models will be useful.

src/Service/SF1500/BrugerService.php Outdated Show resolved Hide resolved
@rimi-itk rimi-itk requested a review from jekuaitk March 22, 2023 08:05
@rimi-itk rimi-itk requested a review from jekuaitk May 2, 2023 10:31
Comment on lines 155 to 157
$organisationFunktioner = $service->soeg([
OrganisationFunktionService::FILTER_FUNKTIONSTYPEID => $this->options['organisation-funktion-manager-id']
]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we end up in a scenario where this list is not exhaustive?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure I understand this question. We want to search for a specific type (id) and therefore search, i.e. filter, only on OrganisationFunktionService::FILTER_FUNKTIONSTYPEID.

Comment on lines 192 to 195
foreach ($registrering->getAttributListe()->getEgenskab() as $egenskab) {
$model->brugernavn = $egenskab->getBrugerNavn();
$model->brugertype = $egenskab->getBrugerTypeTekst();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we unintentionally overwrite brugernavn and brugertype here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not unintentionally, but we assume that we have only a single item in the attribute list. I'll add a clarifying comment.

$model->brugertype = $egenskab->getBrugerTypeTekst();
}
foreach (($registrering->getRelationListe()->getAdresser() ?? []) as $adresse) {
$model->setRelation(
Copy link
Contributor

Choose a reason for hiding this comment

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

setRelation to me makes it seem it is overwritten for each $adresse when in fact it adds to a list of relations. Should we rename to addRelation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add a clarifying comment.

@rimi-itk rimi-itk merged commit d0a9461 into develop May 2, 2023
@rimi-itk rimi-itk deleted the feature/FORMS-674-search branch May 2, 2023 11:40
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