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

Upgrade custom nav #8039

Merged
merged 3 commits into from
Apr 28, 2021
Merged

Conversation

rtibbles
Copy link
Member

@rtibbles rtibbles commented Apr 28, 2021

Summary

  • Allow upgrade of custom nav items
  • Slight refactor of Remote Channel viewset and Remote Channel Resource to return a single item on Model fetch.

Reviewer guidance

Can you upgrade to a newer version of a custom nav channel?

I tested this by upgrading, and then confirming that the new zip file was marked as available in my DB and also that it was my content storage folder.


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

@rtibbles rtibbles requested a review from jonboiser April 28, 2021 22:09
@rtibbles rtibbles added the TODO: needs review Waiting for review label Apr 28, 2021
@rtibbles rtibbles added this to the 0.15.0 milestone Apr 28, 2021
@jonboiser
Copy link
Contributor

jonboiser commented Apr 28, 2021

The tests are failing because the mock of /api/remotechannel still returns arrays.
https://github.com/learningequality/kolibri/pull/8039/checks?check_run_id=2461892526#step:6:423

I don't know if it's possible, but it would create a stronger test to mock the response from Studio itself, rather than mocking the internals of RemoteChannelViewset

@rtibbles rtibbles force-pushed the upgrade_custom_nav branch from 4ae3179 to 585e5cc Compare April 28, 2021 22:40
@rtibbles
Copy link
Member Author

mock the response from Studio itself, rather than mocking the internals of RemoteChannelViewset

From what I can see that is what the test is doing:

def mock_patch_decorator(func):
    def wrapper(*args, **kwargs):
        mock_object = mock.Mock()
        mock_object.json.return_value = [{"id": 1, "name": "studio"}]
        with mock.patch.object(requests, "get", return_value=mock_object):
            return func(*args, **kwargs)

    return wrapper

This is mocking the requests get that happens inside the response, so mocking the Studio response.

@jonboiser
Copy link
Contributor

Ah, I thought mocking requests was somehow affecting the TestCase client, but they are obviously different.

@rtibbles
Copy link
Member Author

Yeah, we (unusually) use requests inside the RemoteChannelViewset to then do the subsequent fetch against Studio.

Copy link
Contributor

@jonboiser jonboiser 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!

@jonboiser jonboiser removed the TODO: needs review Waiting for review label Apr 28, 2021
@jonboiser jonboiser merged commit 98129d4 into learningequality:develop Apr 28, 2021
@jredrejo
Copy link
Member

jredrejo commented May 3, 2021

@rtibbles , @jonboiser the changes in https://github.com/learningequality/kolibri/blob/develop/kolibri/core/content/api.py#L982 is breaking importing channels from studio , due to this commit: 585e5cc

ERROR    Internal Server Error: /api/content/remotechannel/nakav-mafak/retrieve_list/
Traceback (most recent call last):
  File "/datos/le/mio/kolibri/venv/lib/python3.8/site-packages/django/core/handlers/exception.py", line 41, in inner
    response = get_response(request)
  File "/datos/le/mio/kolibri/venv/lib/python3.8/site-packages/django/core/handlers/base.py", line 187, in _get_response
    response = self.process_exception_by_middleware(e, request)
  File "/datos/le/mio/kolibri/venv/lib/python3.8/site-packages/django/core/handlers/base.py", line 185, in _get_response
    response = wrapped_callback(request, *callback_args, **callback_kwargs)
  File "/datos/le/mio/kolibri/venv/lib/python3.8/site-packages/django/views/decorators/csrf.py", line 58, in wrapped_view
    return view_func(*args, **kwargs)
  File "/datos/le/mio/kolibri/venv/lib/python3.8/site-packages/rest_framework/viewsets.py", line 116, in view
    return self.dispatch(request, *args, **kwargs)
  File "/datos/le/mio/kolibri/venv/lib/python3.8/site-packages/rest_framework/views.py", line 497, in dispatch
    self.response = self.finalize_response(request, response, *args, **kwargs)
  File "/datos/le/mio/kolibri/venv/lib/python3.8/site-packages/rest_framework/views.py", line 409, in finalize_response
    assert isinstance(response, HttpResponseBase), (
AssertionError: Expected a `Response`, `HttpResponse` or `HttpStreamingResponse` to be returned from the view, but received a `<class 'list'>`

@rtibbles
Copy link
Member Author

rtibbles commented May 3, 2021

Ah, looks like I forgot to wrap one in a response.

@rtibbles
Copy link
Member Author

rtibbles commented May 3, 2021

Thanks @jredrejo have fixed it here: #8042

@rtibbles rtibbles deleted the upgrade_custom_nav branch May 3, 2021 17:04
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.

3 participants