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

[#2004] Fix order of company branches #942

Merged
merged 1 commit into from
Jan 16, 2024

Conversation

pi-sigma
Copy link
Contributor

@pi-sigma pi-sigma commented Jan 9, 2024

Make sure that the main company branch ("hoofdvestiging") comes before any subsidiary ("nevenvestiging")

Taiga: #2004

@codecov-commenter
Copy link

codecov-commenter commented Jan 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (b52d35e) 94.68% compared to head (0cc8000) 94.69%.
Report is 1 commits behind head on develop.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop     #942   +/-   ##
========================================
  Coverage    94.68%   94.69%           
========================================
  Files          831      831           
  Lines        29275    29297   +22     
========================================
+ Hits         27720    27742   +22     
  Misses        1555     1555           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pi-sigma pi-sigma marked this pull request as ready for review January 10, 2024 08:14
Copy link
Contributor

@stevenbal stevenbal left a comment

Choose a reason for hiding this comment

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

The code looks good 👍

I'm not entirely sure if we're doing the right thing here though. I'm testing locally with the test KVK API and now it looks like this for kvk number 68750110

image

My assumption was that if you selected the hoofdvestiging, you would be able to see all Aanvragen / vragen for all vestigingen. It seems however that the hoofdvestiging does have a vestigingnummer (different from the KVK number), which would mean it would only show the data related to that vestigingsnummer .

The second choice in my screenshot was what I initially thought was the one you'd select if you want to see all data for all branches and that one happens to be of type=rechtspersoon:

"resultaten": [
    {
        "kvkNummer": "68750110",
        "handelsnaam": "Test BV Donald",
        "type": "rechtspersoon",
        "links": [
            {
                "rel": "basisprofiel",
                "href": "https://api.kvk.nl/test/api/v1/basisprofielen/68750110"
            }
        ]
    },
    {
        "kvkNummer": "68750110",
        "vestigingsnummer": "000037178598",
        "handelsnaam": "Test BV Donald",
        "adresType": "bezoekadres",
        "straatnaam": "Hizzaarderlaan",
        "plaats": "Lollum",
        "type": "hoofdvestiging",
        "links": [
            {
                "rel": "basisprofiel",
                "href": "https://api.kvk.nl/test/api/v1/basisprofielen/68750110"
            },
            {
                "rel": "vestigingsprofiel",
                "href": "https://api.kvk.nl/test/api/v1/vestigingsprofielen/000037178598"
            }
        ]
    },
    {
        "kvkNummer": "68750110",
        "vestigingsnummer": "000037178601",
        "handelsnaam": "Test BV Donald Nevenvestiging",
        "adresType": "bezoekadres",
        "straatnaam": "Brinkerinckbaan",
        "plaats": "Diepenveen",
        "type": "nevenvestiging",
        "links": [
            {
                "rel": "basisprofiel",
                "href": "https://api.kvk.nl/test/api/v1/basisprofielen/68750110"
            },
            {
                "rel": "vestigingsprofiel",
                "href": "https://api.kvk.nl/test/api/v1/vestigingsprofielen/000037178601"
            }
        ]
    }
]

I think we should probably do the following:

  1. regardless of whether we see a hoofdvestiging in the KvK data, the first choice should be the one that allows you to see all data (simply the KVK number)
  2. the second choice should be the hoofdvestiging
  3. the rest should be the remaining nevenvestigingen (if they exist) and we should not show any other types like rechtspersoon

@alextreme thoughts?

@alextreme
Copy link
Member

1. regardless of whether we see a `hoofdvestiging` in the KvK data, the first choice should be the one that allows you to see all data (simply the KVK number)

2. the second choice should be the `hoofdvestiging`

3. the rest should be the remaining `nevenvestiging`en (if they exist) and we should not show any other types like `rechtspersoon`

@alextreme thoughts?

That seems like a better approach yes. Sorry, I mixed up the fact that the 'KvK number without branch selection' (so for acting as the company as a whole) is something different than selecting the hoofdvestiging, which is a branch

@pi-sigma
Copy link
Contributor Author

@alextreme @stevenbal What about the case where the response only contains branches with a vestigingsnummer? For example

[
  {
    "kvkNummer": "69599084",
    "vestigingsnummer": "000038509504",
    "handelsnaam": "Test EMZ Dagobert",
    "adresType": "bezoekadres",
    "straatnaam": "Abebe Bikilalaan",
    "plaats": "Amsterdam",
    "type": "hoofdvestiging",
    "links": [
      {
        "rel": "basisprofiel",
        "href": "https://api.kvk.nl/test/api/v1/basisprofielen/69599084"
      },
      {
        "rel": "vestigingsprofiel",
        "href": "https://api.kvk.nl/test/api/v1/vestigingsprofielen/000038509504"
      }
    ]
  },
  {
    "kvkNummer": "69599084",
    "vestigingsnummer": "000038509520",
    "handelsnaam": "Test EMZ Nevenvestiging Govert",
    "adresType": "bezoekadres",
    "straatnaam": "Geneinde",
    "plaats": "Maastricht",
    "type": "nevenvestiging",
    "links": [
      {
        "rel": "basisprofiel",
        "href": "https://api.kvk.nl/test/api/v1/basisprofielen/69599084"
      },
      {
        "rel": "vestigingsprofiel",
        "href": "https://api.kvk.nl/test/api/v1/vestigingsprofielen/000038509520"
      }
    ]
  }
]

Given our current setup, the user won't be able to log in and retrieve information about the entire company. Should we check for this case and create an additional option (i.e. without vestiging restriction), or display only the options returned by the KvK API?

@stevenbal
Copy link
Contributor

@pi-sigma I would do it like this:

  • modify get_all_company_branches to only return entries with type hoofdvestiging or nevenvestiging
  • use those branches in the form used for branch selection and always add an option that simply uses the KVK number (so no branch number) as the first option. This way we don't have to check the options returned by the KVK API and we're sure that the entire company can always be selected

@pi-sigma pi-sigma force-pushed the fix/2004-main-company-branch branch 2 times, most recently from f7c9c6e to 1d78364 Compare January 12, 2024 17:59
src/open_inwoner/accounts/tests/test_auth.py Outdated Show resolved Hide resolved
src/open_inwoner/accounts/tests/test_auth.py Outdated Show resolved Hide resolved
src/open_inwoner/kvk/client.py Outdated Show resolved Hide resolved
src/open_inwoner/kvk/tests/test_api.py Show resolved Hide resolved
src/open_inwoner/kvk/views.py Outdated Show resolved Hide resolved
@pi-sigma pi-sigma force-pushed the fix/2004-main-company-branch branch 4 times, most recently from 1cd8a25 to 1f12b9e Compare January 16, 2024 10:09
@stevenbal stevenbal self-requested a review January 16, 2024 11:37
Copy link
Contributor

@stevenbal stevenbal left a comment

Choose a reason for hiding this comment

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

Looks good now, the only remaining thing is the incorrect rebase (which also causes the failing tests)

src/open_inwoner/kvk/client.py Show resolved Hide resolved
@stevenbal stevenbal removed the request for review from alextreme January 16, 2024 14:05
@alextreme alextreme merged commit 3321179 into develop Jan 16, 2024
14 checks passed
@alextreme alextreme deleted the fix/2004-main-company-branch branch January 16, 2024 17:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants