Merge pull request #1533 from maykinmedia/task/2931-update-company-login

[#2931] Update company name from KVK API on login

src/open_inwoner/accounts/signals.py

@@ -8,6 +8,7 @@
 from open_inwoner.accounts.models import User
 from open_inwoner.haalcentraal.models import HaalCentraalConfig
 from open_inwoner.haalcentraal.utils import update_brp_data_in_db
+from open_inwoner.kvk.client import KvKClient
 from open_inwoner.openklant.services import OpenKlant2Service, eSuiteKlantenService
 from open_inwoner.utils.logentry import user_action
 
@@ -26,7 +27,7 @@
 
 
 @receiver(user_logged_in)
-def update_user_from_klant_on_login(sender, user, request, *args, **kwargs):
+def update_user_on_login(sender, user, request, *args, **kwargs):
     # This additional guard is mainly to facilitate easier testing, where not
     # all request factories return a full-fledged request object.
     if not hasattr(request, "user"):
@@ -35,6 +36,9 @@ def update_user_from_klant_on_login(sender, user, request, *args, **kwargs):
     if user.login_type not in [LoginTypeChoices.digid, LoginTypeChoices.eherkenning]:
         return
 
+    if user.login_type is LoginTypeChoices.eherkenning:
+        _update_eherkenning_user_from_kvk_api(user=user)
+
     # OpenKlant2
     try:
         service = OpenKlant2Service()
@@ -74,6 +78,16 @@ def _update_user_from_esuite(
         service.update_user_from_klant(klant, user)
 
 
+def _update_eherkenning_user_from_kvk_api(user: User):
+    kvk_client = KvKClient()
+
+    vestiging = kvk_client.get_company_headquarters(kvk=user.kvk)
+
+    if company_name := vestiging.get("naam"):
+        user.company_name = company_name
+        user.save()
+
+
 @receiver(user_logged_in)
 def log_user_login(sender, user, request, *args, **kwargs):
     current_path = request.path

src/open_inwoner/accounts/tests/test_auth.py

@@ -14,7 +14,7 @@
 from pyquery import PyQuery as PQ
 
 from open_inwoner.accounts.choices import NotificationChannelChoice
-from open_inwoner.accounts.signals import update_user_from_klant_on_login
+from open_inwoner.accounts.signals import KvKClient, update_user_on_login
 from open_inwoner.configurations.models import SiteConfiguration
 from open_inwoner.haalcentraal.tests.mixins import HaalCentraalMixin
 from open_inwoner.kvk.branches import get_kvk_branch_number
@@ -1896,7 +1896,7 @@ def test_anonymous_user_is_redirected_to_login_page_if_password_change_is_access
 
 
 @requests_mock.Mocker()
-class TestUpdateUserFromKlantUponLoginTests(TestCase):
+class UpdateUserOnLoginTest(TestCase):
     @classmethod
     def setUpTestData(cls):
         MockAPIReadPatchData.setUpServices()
@@ -1906,31 +1906,86 @@ def setUpTestData(cls):
 
     def test_update_hook_is_registered_on_login(self, m):
         connected_functions = [receiver[1]() for receiver in user_logged_in.receivers]
-        self.assertIn(update_user_from_klant_on_login, connected_functions)
+        self.assertIn(update_user_on_login, connected_functions)
 
-    def test_update_user_from_klant_hook_only_called_for_digid_and_eherkenning(self, m):
+    def test_update_hook_not_called(self, m):
         self.data = MockAPIReadPatchData().install_mocks(m)
         request = RequestFactory().get("/foo")
         request.user = self.data.user
 
-        for login_type in LoginTypeChoices:
-            with self.subTest(
-                f"Test update klant hook is called for login type {login_type}"
-            ):
+        for login_type in [LoginTypeChoices.default, LoginTypeChoices.oidc]:
+            with self.subTest(f"{login_type}"):
                 self.data.user.login_type = login_type
                 self.data.user.save()
+
+                update_user_on_login(
+                    self.__class__,
+                    request.user,
+                    request,
+                )
+
                 with patch(
                     "open_inwoner.openklant.services.eSuiteKlantenService.update_user_from_klant"
-                ) as update_user_from_klant_mock:
-                    update_user_from_klant_on_login(
-                        self.__class__,
-                        request.user,
-                        request,
-                    )
-                    if login_type in [
-                        LoginTypeChoices.digid,
-                        LoginTypeChoices.eherkenning,
-                    ]:
-                        update_user_from_klant_mock.assert_called_once()
-                    else:
-                        update_user_from_klant_mock.assert_not_called()
+                ) as update_user_mock:
+                    update_user_mock.assert_not_called()
+
+    def test_digid_user_update_hook_called(self, m):
+        self.data = MockAPIReadPatchData().install_mocks(m)
+        request = RequestFactory().get("/foo")
+        request.user = self.data.user
+
+        self.data.user.login_type = LoginTypeChoices.digid
+        self.data.user.save()
+        with patch(
+            "open_inwoner.openklant.services.eSuiteKlantenService.update_user_from_klant"
+        ) as update_user_mock:
+            update_user_on_login(
+                self.__class__,
+                request.user,
+                request,
+            )
+            update_user_mock.assert_called_once()
+
+    def test_update_eherkenning_user(self, m):
+        self.data = MockAPIReadPatchData().install_mocks(m)
+        request = RequestFactory().get("/foo")
+        request.user = self.data.user
+
+        self.data.user.login_type = LoginTypeChoices.eherkenning
+        self.data.user.save()
+
+        self.assertEqual(request.user.company_name, "")
+
+        vestiging = {
+            "kvkNummer": "68750110",
+            "vestigingsnummer": "000037178598",
+            "naam": "Test BV Donald",
+            "adres": {
+                "binnenlandsAdres": {
+                    "type": "bezoekadres",
+                    "straatnaam": "Hizzaarderlaan",
+                    "plaats": "Lollum",
+                }
+            },
+            "type": "hoofdvestiging",
+            "_links": {
+                "basisprofiel": {
+                    "href": "https://api.kvk.nl/test/api/v1/basisprofielen/68750110"
+                },
+                "vestigingsprofiel": {
+                    "href": "https://api.kvk.nl/test/api/v1/vestigingsprofielen/000037178598"
+                },
+            },
+        }
+
+        with patch.object(KvKClient, "get_company_headquarters") as mock_kvk:
+            mock_kvk.return_value = vestiging
+            update_user_on_login(
+                self.__class__,
+                request.user,
+                request,
+            )
+
+            mock_kvk.assert_called_once()
+
+        self.assertEqual(request.user.company_name, "Test BV Donald")

src/open_inwoner/kvk/tests/test_views.py

@@ -152,14 +152,23 @@ def test_get_branches_page_no_branches_found_sets_branch_check_done(
     @patch(
         "open_inwoner.kvk.models.KvKConfig.get_solo",
     )
-    def test_get_branches_page_one_branch_found_sets_branch_check_done(
-        self, mock_solo, mock_kvk
-    ):
+    def test_get_branches_page_one_branch_found(self, mock_solo, mock_kvk):
         """
-        Regression test for endless redirect: https://taiga.maykinmedia.nl/project/open-inwoner/task/2000
+        The branch selection page should be displayed, and the vestigingsnummer stored in the
+        session, even if only one branch is found.
+
+        Taiga: https://taiga.maykinmedia.nl/project/open-inwoner/task/2946
+
+        We previously skipped the branch selection for single-branch companies because of problems
+        with our redirect middleware: https://taiga.maykinmedia.nl/project/open-inwoner/task/2000
         """
         mock_kvk.return_value = [
-            {"kvkNummer": "12345678", "vestigingsnummer": "1234"},
+            {
+                "kvkNummer": "12345678",
+                "vestigingsnummer": "1234",
+                "naam": "Makers and Shakers",
+                "type": "hoofdvestiging",
+            },
         ]
 
         mock_solo.return_value.api_key = "123"
@@ -171,17 +180,24 @@ def test_get_branches_page_one_branch_found_sets_branch_check_done(
 
         response = self.client.get(self.url)
 
-        self.assertEqual(response.status_code, 302)
-        self.assertEqual(response.url, reverse("pages-root"))
-        # Because no branches were found, the branch check should be skipped in the future
-        # and no branch number should be set
-        self.assertEqual(kvk_branch_selected_done(self.client.session), True)
-        self.assertEqual(get_kvk_branch_number(self.client.session), None)
+        self.assertEqual(response.status_code, 200)
 
-        response = self.client.get(response.url)
+        doc = PyQuery(response.content)
 
-        # Following redirect should not result in endless redirect
-        self.assertEqual(response.status_code, 200)
+        branch_inputs = doc.find("[name='branch_number']")
+
+        # check that pseudo-branch representing company as a whole has been added
+        self.assertEqual(len(branch_inputs), 2)
+
+        self.assertEqual(branch_inputs[0], doc.find("[id='entire-company']")[0])
+        self.assertEqual(branch_inputs[1], doc.find("[id='branch-1234']")[0])
+
+        # chack that company name is displayed for every branch
+        company_name_displays = doc("h2:Contains('Makers and Shakers')")
+        self.assertEqual(len(company_name_displays), 2)
+
+        main_branch_display = doc("p:Contains('Hoofdvestiging')")
+        self.assertEqual(len(main_branch_display), 1)
 
     @patch("open_inwoner.kvk.client.KvKClient.get_all_company_branches")
     @patch(

src/open_inwoner/kvk/views.py

@@ -32,7 +32,9 @@ def get_form_kwargs(self):
         company_branches = kvk_client.get_all_company_branches(
             kvk=self.request.user.kvk
         )
-        # create pseudo-branch representing the company as a whole
+        # create pseudo-branch representing the company as a whole.
+        # technically, the compnay as a legal entity is represented as "rechtspersoon",
+        # but this is not always included in query results
         master_branch = {
             "vestigingsnummer": "",
             "naam": company_branches[0].get("naam", "") if company_branches else "",
@@ -63,11 +65,9 @@ def get(self, request, *args, **kwargs):
 
         form = context["form"]
 
-        company_branches_with_master = form.company_branches
-        # Exclude the "master" branch from these checks, since we always insert this
-        company_branches = company_branches_with_master[1:]
-
-        if not company_branches or len(company_branches) == 1:
+        # check that there are company branches besides our artifical "master_branch"
+        vestigingen = form.company_branches[1:]
+        if not vestigingen or not any(v.get("vestigingsnummer") for v in vestigingen):
             request.session[KVK_BRANCH_SESSION_VARIABLE] = None
             request.session.save()
             return HttpResponseRedirect(redirect)

src/open_inwoner/openklant/tests/test_views.py

@@ -14,7 +14,7 @@
 from pyquery import PyQuery
 from zgw_consumers.api_models.base import factory
 
-from open_inwoner.accounts.signals import update_user_from_klant_on_login
+from open_inwoner.accounts.signals import update_user_on_login
 from open_inwoner.accounts.tests.factories import DigidUserFactory, UserFactory
 from open_inwoner.configurations.models import SiteConfiguration
 from open_inwoner.openklant.api_models import ContactMoment, Klant, KlantContactMoment
@@ -60,7 +60,7 @@ def setUp(self):
         )
 
         super().setUp()
-        signals.user_logged_in.disconnect(receiver=update_user_from_klant_on_login)
+        signals.user_logged_in.disconnect(receiver=update_user_on_login)
 
         MockAPIReadData.setUpServices()
         self.api_group = ZGWApiGroupConfig.objects.get()