Replacing requests with NetworkClient

[thesujai]

Incomplete, Just had some clarification questions

Summary

This replaces the usage of requests with NetworkClient

References

Fixes #11018

Reviewer guidance

---

Testing checklist

• [x] Contributor has fully tested the PR manually
• [ ] If there are any front-end changes, before/after screenshots are included
• [ ] Critical user journeys are covered by Gherkin stories
• [ ] Critical and brittle code paths are covered by unit tests

PR process

• [x] PR has the correct target branch and milestone
• [x] PR has 'needs review' or 'work-in-progress' label
• [ ] If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
• [ ] If this is an important user-facing change, PR or related issue has a 'changelog' label
• [ ] If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

• Automated test coverage is satisfactory
• PR is fully functional
• PR has been tested for accessibility regressions
• External dependency files were updated if necessary (yarn and pip)
• Documentation is updated
• Contributor is in AUTHORS.md

[github-actions[bot]]

Build Artifacts

Asset type	Download link
PEX file	kolibri-.pex
Windows Installer (EXE)	kolibri-0.17.0b1.dev0+git.32.g0943be96-windows-setup-unsigned.exe
Debian Package	kolibri_0.17.0b1.dev0+git.32.g0943be96-0ubuntu1_all.deb
Mac Installer (DMG)	kolibri-0.17.0b1.dev0+git.32.g0943be96.dmg
Android Package (APK)	kolibri-0.17.0b1.dev0+git.32.g0943be96-0.1.3-debug.apk
TAR file	kolibri-0.17.0b1.dev0+git.32.g0943be96.tar.gz
WHL file	kolibri-0.17.0b1.dev0+git.32.g0943be96-py2.py3-none-any.whl

[thesujai]

Not so straight forward as I thought:) I replaced requests in core/content/api.py then arround 30 testcases were failing, they were KolibriStudioAPITestCase and ProxyContentMetadataTestCase. Initially i tried directly mocking NetworkClient like below:

@mock.patch("kolibri.core.content.api.NetworkClient")
class KolibriStudioAPITestCase(APITestCase):
    @classmethod
    def setUpTestData(cls):
        DeviceSettings.objects.create(is_provisioned=True)
        cls.facility = Facility.objects.create(name="facility")
        superuser = FacilityUser.objects.create(
            username="superuser", facility=cls.facility
        )
        superuser.set_password(DUMMY_PASSWORD)
        superuser.save()
        cls.superuser = superuser
        DevicePermissions.objects.create(user=superuser, is_superuser=True)

    def setUp(self):
        self.client.login(username=self.superuser.username, password=DUMMY_PASSWORD)

    def test_channel_list(self, networkclient_mock):
        mock_response = networkclient_mock.build_for_address.return_value.get.return_value
        mock_response.json.return_value = [{"id": 1, "name": "studio"}]
        mock_response.status_code = 200

        response = networkclient_mock.build_for_address.return_value.get(
            reverse("kolibri:core:remotechannel-list"), format="json"
        )
        self.assertEqual(response.status_code, 200)
        self.assertEqual(response.json()[0]["id"], 1)

    def test_no_permission_non_superuser_channel_list(self, networkclient_mock):
        user = FacilityUser.objects.create(username="user", facility=self.facility)
        user.set_password(DUMMY_PASSWORD)
        user.save()
        self.client.logout()
        self.client.login(username=user.username, password=DUMMY_PASSWORD)

        mock_response = networkclient_mock.build_for_address.return_value.get.return_value
        mock_response.status_code = 403

        response = networkclient_mock.build_for_address.return_value.get(
            reverse("kolibri:core:remotechannel-list"), format="json"
        )
        self.assertEqual(response.status_code, 403)

    def test_channel_retrieve(self, networkclient_mock):
        mock_response = networkclient_mock.build_for_address.return_value.get.return_value
        mock_response.json.return_value = [{"id": 1, "name": "studio"}]
        mock_response.status_code = 200

        response = networkclient_mock.build_for_address.return_value.get(
            reverse("kolibri:core:remotechannel-list"), format="json"
        )
        self.assertEqual(response.status_code, 200)
        self.assertEqual(response.json(), [{"id": 1, "name": "studio"}])

    def test_channel_info_404(self, networkclient_mock):
        mock_response = networkclient_mock.build_for_address.return_value.get.return_value
        mock_response.status_code = 404

        response = self.client.get(
            reverse("kolibri:core:remotechannel-detail", kwargs={"pk": "abc"}),
            format="json",
        )
        self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND)

    def test_channel_info_offline(self, networkclient_mock):
        mock_response = networkclient_mock.build_for_address.return_value.get.return_value
        mock_response.status_code = 503
        mock_response.json.return_value = {"status": "offline"}

        response = networkclient_mock.build_for_address.return_value.get(
            reverse("kolibri:core:remotechannel-detail", kwargs={"pk": "abc"}),
            format="json",
        )
        self.assertEqual(response.status_code, status.HTTP_503_SERVICE_UNAVAILABLE)
        self.assertEqual(response.json()["status"], "offline")

    def test_channel_list_offline(self, networkclient_mock):
        mock_response = networkclient_mock.build_for_address.return_value.get.return_value
        mock_response.status_code = 503
        mock_response.json.return_value = {"status": "offline"}

        response = networkclient_mock.build_for_address.return_value.get(
            reverse("kolibri:core:remotechannel-list"), format="json"
        )
        self.assertEqual(response.status_code, status.HTTP_503_SERVICE_UNAVAILABLE)
        self.assertEqual(response.json()["status"], "offline")

    def tearDown(self):
        cache.clear()

These were passing but then what is the use of client.login etc if they are not being used, so i dropped this approach.

Are there other ways to do this?

[rtibbles]

Not sure if I am completely understanding your question, but I think using a mock for the NetworkClient does seem like a good idea - the alternative would be to use something like https://requests-mock.readthedocs.io/en/latest/ - but maybe that's best used for the NetworkClient tests specifically, and then we mock the NetworkClient here.

It does seem that being able to distinguish a 404 response from other responses might be useful? As the NetworkLocationResponseFailure has the requests response attached to it as the response attribute, we can still make assertions about the status_code of that response to check for a 404.

[thesujai]

what i actually wanted to ask is that - if you see this test case you will see in the setUp we are authenticating the client here But in the modified testcase i provided, I am trying to mock networkclient and do api testing, but i am not able to perform the authentication like it was doing before.

So how do i leverage the authentication(APITestCase class provided client.login()) and at the same time mock NetworkClient

[rtibbles]

So how do i leverage the authentication(APITestCase class provided client.login()) and at the same time mock NetworkClient

Maybe I am missing something, but these are two separate things. The NetworkClient is only ever being used to make calls to another Kolibri, either within the course of an API request, or inside an asynchronous task. The test client used in the test cases is used to make requests to API endpoints in an efficient way (it actually kind of cheats and bypasses the http layer). So, you can still login to the test client to make requests to API endpoints, then if inside that API endpoint it is using NetworkClient to make a further request to an external Kolibri, that's where the mocking of the NetworkClient comes into play, because now instead of reaching out to the remote Kolibri (or Kolibri Studio) we just provide a mocked response from the NetworkClient.

This is good, as this is one source of flakiness for our automated tests, where if Kolibri Studio is slow to respond (either because of Studio or local network conditions), it can cause a false positive test failure.

[MisRob]

Hi @thesujai, is this ready for re-review or work in progress?

[thesujai]

Hello @MisRob Can you mark this as work-in-progress? I have the changes made but it is kind of messed up so I haven't pushed it to this branch yet.

[MisRob]

Yes, done. Thank you.

[thesujai]

This is ready for review!

[MisRob]

Thanks @thesujai

[rtibbles]

Have confirmed I see no issues with telemetry pingback.

[rtibbles]

@radinamatic @pcenov to confirm no regressions here, I think the main things to regression test here are:

• LoD resource syncing for lessons and quizzes
• Registering and syncing with KDP
• Importing resources from another Kolibri
• Creating a user on a remote facility during the change facility workflow and setup wizard single learner import flows

[pcenov]

Hi @thesujai @rtibbles, After fully regression testing the above mentioned user flows I can confirm that everything seems to be functioning correctly with the exception of the change facility workflows where I am seeing 403 and 500 errors in the console at the 'Remotely integrating data' step and it takes about 3-5 minutes for the completion of the migration (initially I thought that it's not working at all). This issue is not extant in the official Kolibri 0.17 build.

Here are 3 videos (which I have shortened) with different scenarios illustrating that.

1. Merge an existing learner:

https://github.com/user-attachments/assets/e23d0d32-fd8d-48fa-b496-561e09fcb92c

Logs: mergeUserLogs.zip

2. Create a new user:

https://github.com/user-attachments/assets/ce7a4733-b11d-45f7-ba6d-bd4eae6f09d0

Logs: createNewUserLogs.zip

3. Move account:

https://github.com/user-attachments/assets/1b62a545-20c3-4049-a570-4d0959e6f68b

Logs: moveAccountLogs.zip

[rtibbles]

Thanks @pcenov - I will double check the logs you shared and confirm whether this is caused by this PR or whether it is something that has been independently fixed, and this PR is not up to date with!

[rtibbles]

Yes - it looks like these errors and the apparently stalling migration are all things that were independently fixed in the release-v0.17.x branch that have not been merged up to here, so I think this is good to go. None of the endpoints that were modified in this PR are causing any issues!