Add regression testing for channel update deletion behaviour

[thesujai]

Summary

Add test cases as mentioned in https://github.com/learningequality/kolibri/issues/5960#issuecomment-1937101674 Also handle the case when current_version or current_partial is None, preventing the TypeError from occurring. …

References

Fixes #5960 …

Reviewer guidance

I am not sure about my test_full_import_with_newer_version which apparently is failing …

---

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
• [x] Critical and brittle code paths are covered by unit tests

PR process

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

[github-actions[bot]]

Build Artifacts

Asset type	Download link
PEX file	kolibri-0.16.0a0.dev0_git.591.g54dce2d1.pex
Windows Installer (EXE)	kolibri-0.16.0a0.dev0+git.591.g54dce2d1-unsigned.exe
Debian Package	kolibri_0.16.0a0.dev0+git.591.g54dce2d1-0ubuntu1_all.deb
Mac Installer (DMG)	kolibri-0.16.0a0.dev0+git.591.g54dce2d1-0.4.0.dmg
Android Package (APK)	kolibri-0.16.0a0.dev0+git.591.g54dce2d1-0.1.0-debug.apk
TAR file	kolibri-0.16.0a0.dev0+git.591.g54dce2d1.tar.gz
WHL file	kolibri-0.16.0a0.dev0+git.591.g54dce2d1-py2.py3-none-any.whl

[thesujai]

@rtibbles for test_full_import_with_newer_version i am getting following error:

def raise_(
    exception, with_traceback=None, replace_context=None, from_=False
):
    r"""implement "raise" with cause support.

        :param exception: exception to raise
        :param with_traceback: will call exception.with_traceback()
        :param replace_context: an as-yet-unsupported feature.  This is
         an exception object which we are "replacing", e.g., it's our
         "cause" but we don't want it printed.    Basically just what
         ``__suppress_context__`` does but we don't want to suppress
         the enclosing context, if any.  So for now we make it the
         cause.
        :param from\_: the cause.  this actually sets the cause and doesn't
         hope to hide it someday.

        """
    if with_traceback is not None:
        exception = exception.with_traceback(with_traceback)

    if from_ is not False:
        exception.__cause__ = from_
    elif replace_context is not None:
        # no good solution here, we would like to have the exception
        # have only the context of replace_context.__context__ so that the
        # intermediary exception does not change, but we can't figure
        # that out.
        exception.__cause__ = replace_context

    try:
       raise exception

E sqlalchemy.exc.ArgumentError: SQL expression for WHERE/HAVING role expected, got <MagicMock name='Bridge().get_table().c.contentnode_id.in_()' id='140025039648992'>.

../../../.pyenv/versions/3.9.9/envs/kolibriNew/lib/python3.9/site-packages/sqlalchemy/util/compat.py:211: ArgumentError ============================================ 1 failed, 4 passed in 16.73 seconds ============================================

[rtibbles]

My first suggestion here would be to use the TransactionTestCase in the same way as the other test cases in this file. This is because we are using SQLAlchemy in the ChannelImport logic, but the regular TestCase doesn't play nicely with that.

It may be necessary to subclass the ContentImportBase class for this, to do additional setup and mocking: https://github.com/learningequality/kolibri/pull/11896/files#diff-192861a164181b5a5a8f25abf4bfa8502039d501ad331a5b24b7b149cb0d6f41R381

[thesujai]

Thanks for the review! I made the suggested changes, let me know if the tests are serving the purpose.

I also have a confusion that in this elif condition https://github.com/learningequality/kolibri/blob/develop/kolibri/core/content/utils/channel_import.py#L744 shouldn't we return a False?

[rtibbles]

I also have a confusion that in this elif condition https://github.com/learningequality/kolibri/blob/develop/kolibri/core/content/utils/channel_import.py#L744 shouldn't we return a False?

No - in that elif condition we are deleting the currently imported channel metadata, so we need to return True. If we returned False, we would delete the existing metadata, and then not import the updated metadata.

[thesujai]

Okay thanks

[rtibbles]

Merging with the broken docs build, as this doesn't appear to be caused by this PR.