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

[#2026] KVK branch selection #1043

Merged
merged 4 commits into from
Mar 4, 2024
Merged

Conversation

@jiromaykin jiromaykin changed the title [#2026] First commit KVK branch selection [#2026] KVK branch selection Feb 20, 2024
@jiromaykin jiromaykin force-pushed the feature/2026-kvk-branch-design branch from 641f262 to 9064364 Compare February 22, 2024 10:36
@jiromaykin jiromaykin added the wip Work in progress label Feb 22, 2024
@codecov-commenter
Copy link

codecov-commenter commented Feb 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.99%. Comparing base (53a9201) to head (de88960).
Report is 12 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1043      +/-   ##
===========================================
+ Coverage    94.95%   94.99%   +0.03%     
===========================================
  Files          892      892              
  Lines        31203    31375     +172     
===========================================
+ Hits         29630    29805     +175     
+ Misses        1573     1570       -3     

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

@jiromaykin jiromaykin force-pushed the feature/2026-kvk-branch-design branch 7 times, most recently from 442ff6d to f550845 Compare February 29, 2024 14:17
@jiromaykin jiromaykin marked this pull request as ready for review February 29, 2024 14:38
@jiromaykin jiromaykin removed the wip Work in progress label Feb 29, 2024
@jiromaykin
Copy link
Contributor Author

@pi-sigma @stevenbal with this one I am not quite sure how to deal with the KVK data, since I cannot retrieve a 'housenumber' - also I am not quite sure if there are companies that do not have ANY Hoofdvestiging at all?

If I show all branches including the Hoofdvestiging, then at least the tests are passing through...

I commented out the template for separating the Hoofdvestiging from all the Nevenvestiging branches since that would need a rework for the tests + I am not sure how to combine both options logically
(1. only show all branches in 1 list if there is no Hoofdvestiging,
2. IF Hoofdvestiging exists: show a separate List-item for Hoofdvestiging and group only the List-items for Nevenvestigingen),
here you can see the differences between both:

which-branches

Copy link
Contributor

@pi-sigma pi-sigma left a comment

Choose a reason for hiding this comment

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

I'll take a closer look later/tomorrow and see if we need to make changes in the backend in order to support the split template.

About the house number/letter and other data: is this an explicit request from the client or something the designer(s) assumed would make sense?

{% endfor %}


{# first only show HOOFDVESTIGING #}
Copy link
Contributor

Choose a reason for hiding this comment

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

The branch without vestigingsnummer is not the hoofdvestging, but a pseudo-branch that we create to represent the company as a whole. The hoofdvestiging is one vestiging among others, and as such it also has a vestigingsnummer.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's right, maybe we should define the "entire company" option outside of company_branches to avoid confusion? Or would that make the form more complicated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pi-sigma I've left he commented-out code for the split-template still in it, for future use ;
It's all inside one large unordered-list.
feel free to approve this one and/or make changes in this PR itself.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jiromaykin I could modify it to use the split template, I'll approve and assign the PR to myself 👍

@jiromaykin
Copy link
Contributor Author

... About the house number/letter and other data: is this an explicit request from the client or something the designer(s) assumed would make sense?

I am just trying to mimic the design - and I don't know how much of it is a 'hard requirement' - perhaps @alextreme knows more about what to include.

{% endfor %}


{# first only show HOOFDVESTIGING #}
Copy link
Contributor

Choose a reason for hiding this comment

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

That's right, maybe we should define the "entire company" option outside of company_branches to avoid confusion? Or would that make the form more complicated?

<div class="choice-list__content">
<h2 class="choice-list__heading">{{ branch.naam }}</h2>
{% if branch.straatnaam %}
<p class="choice-list__p">{{ branch.straatnaam }}</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

It does seem there is some location info being passed, you could access it with {{ branch.adres.binnenlandsAdres.straatnaam }} but it doesn't have huisnummers unfortunately:


   {
      "adres":{
         "binnenlandsAdres":{
            "plaats":"Lollum",
            "straatnaam":"Hizzaarderlaan",
            "type":"bezoekadres"
         }
      },
      "kvkNummer":"68750110",
      "links":[
         {
            "href":"https://api.kvk.nl/test/api/v1/basisprofielen/68750110",
            "rel":"basisprofiel"
         },
         {
            "href":"https://api.kvk.nl/test/api/v1/vestigingsprofielen/000037178598",
            "rel":"vestigingsprofiel"
         }
      ],
      "naam":"Test BV Donald",
      "type":"hoofdvestiging",
      "vestigingsnummer":"000037178598"
   },
   

@pi-sigma if we want the full address for each vestiging we might have to do separate calls to /v1/vestigingsprofielen/<vestigingsnummer>?

@stevenbal
Copy link
Contributor

stevenbal commented Feb 29, 2024

with this one I am not quite sure how to deal with the KVK data, since I cannot retrieve a 'housenumber' - also I am not quite sure if there are companies that do not have ANY Hoofdvestiging at all?

@jiromaykin I think that every company in the KVK data has a hoofdvestiging with its own vestigingsnummer different from the KVK nummer (even if it is the only vestiging)

I feel like it would be a bit unnecessary to display both the "entire company" option and the hoofdvestiging option, if there are no other vestigingen though

@alextreme
Copy link
Member

... About the house number/letter and other data: is this an explicit request from the client or something the designer(s) assumed would make sense?

I am just trying to mimic the design - and I don't know how much of it is a 'hard requirement' - perhaps @alextreme knows more about what to include.

@jiromaykin showing the adres of a vestiging isn't a requirement, if not (easily) possible please ask Jacki to modify the design accordingly

@jiromaykin jiromaykin force-pushed the feature/2026-kvk-branch-design branch from f550845 to 18b60ee Compare February 29, 2024 17:03
@stevenbal stevenbal self-assigned this Mar 1, 2024
@stevenbal stevenbal marked this pull request as draft March 1, 2024 08:53
@stevenbal
Copy link
Contributor

I modified the template to split the "entire company" option from the other branches, added the adress info (without housenumber, because that's not present in the data) and added (hoofdvestiging) to indicate that the first branch option is the hoofdvestiging:

image

Nevenvestiging is part of the name of the branch in the test KVK API, so that can be ignored

@stevenbal stevenbal requested a review from alextreme March 1, 2024 09:56
@stevenbal stevenbal removed their assignment Mar 1, 2024
@stevenbal stevenbal self-requested a review March 1, 2024 09:57
@stevenbal stevenbal marked this pull request as ready for review March 1, 2024 09:57
@stevenbal stevenbal force-pushed the feature/2026-kvk-branch-design branch from 6a59b71 to e437fd8 Compare March 1, 2024 10:11
@alextreme alextreme requested review from Bartvaderkin and removed request for alextreme March 4, 2024 09:27
@jiromaykin jiromaykin requested a review from pi-sigma March 4, 2024 11:52
@stevenbal stevenbal merged commit 1eb0750 into develop Mar 4, 2024
15 checks passed
@stevenbal stevenbal deleted the feature/2026-kvk-branch-design branch March 4, 2024 12:57
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.

6 participants