Replace requests with NetworkClient

[thesujai]

Summary

• Use NetworkClient to make HTTP requests instead of requests.
• Use the NetworkClient errors in place of regular requests exception, as the NetworkClient Errors are wrapper around them
• Update the mocks of requests with NetworkClient in tests

References

Fixes #11018

Reviewer guidance

I did a manual test thoroughly, and mostly followed TDD.
The only place I am not sure is core.api.KolibriDataPortalViewSet as testing it required a token and no tests are written for this. Maybe we can have a issue to do this?

---

Testing checklist

• 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

• PR has the correct target branch and milestone
• 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

[thesujai]

Are we supposed to do this? Because here, requests is only used for getting the Error Class

[thesujai]

Initially i thought NetworkClient is not working properly as for this endpoint i was getting a 404, but with requests it was going smoothly for the same endpoint. Turns out there were no exception handling for 404(directly using requests).
I think endpoint should return a 400 if the token is invalid(or NONE) instead of a 404???

[rtibbles]

Looks like you could just set:
response = e.response here and let the code continue?

[github-actions]

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?

[thesujai]

I found some places in code using requests that the baseurl is none, so to migrate to NetworkClient without raising any error, I did this.

[rtibbles]

Are these the cases where the baseurl is the server itself? Maybe for that case we should make a build_for_localhost method or something to capture this case?

[thesujai]

By server you mean https://studio.learningequality.org ?
One place i can recall where i observed the baseurl is None is this, this happens when from the browser only we try to import channel from studio.
So this behaviour(base being NONE) is already handled in get_content_server_url which our _make_channel_endpoint_request eventually calls to stack.

So for these cases instead of building network client from build_for_address which will throw an error is address is None. Can we simply create a NetworkClient obj as:

client = NetworkClient(baseurl)  # it deals with baseurl being null right?

[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]

All the code changes here make sense to me - I am not concerned about the KDP interactions, but wonder if this might mess with telemetry reporting. Will be checking locally.

[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:

merge.user.mp4

Logs: mergeUserLogs.zip

2. Create a new user:

create.new.user.mp4

Logs: createNewUserLogs.zip

3. Move account:

Move.account.mp4

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!

[rtibbles]

Code review passed, manual QA has shown no regressions from these changes.