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

Pass the correct kwargs to the cleanupsyncs management command. #11854

Conversation

rtibbles
Copy link
Member

@rtibbles rtibbles commented Feb 10, 2024

Summary

  • The morango cleanupsyncs command takes push and pull kwargs to specify push and pull cleanups, but our task wrapper is currently passing is_push and is_pull
  • Rectifies this by correcting the kwargs after the validation has happened
  • Updates the relevant test

References

No extant issue - noticed while doing some testing on Django 3.2 upgrading, where Django now complains if you pass non-existent kwargs to a management command!

Reviewer guidance

I ran the tests, with the tweak they passed.


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 bjester February 10, 2024 23:23
@rtibbles rtibbles added the TODO: needs review Waiting for review label Feb 10, 2024
@github-actions github-actions bot added DEV: backend Python, databases, networking, filesystem... SIZE: small labels Feb 10, 2024
Copy link
Member

@bjester bjester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it might be better to pass in the correct arguments to begin with, here https://github.com/learningequality/kolibri/blob/develop/kolibri/core/auth/kolibri_plugin.py#L54-L55

@rtibbles
Copy link
Member Author

Sure - I was going for the fewest changes, but if it's fine to change the function signature, then happy to do that.

@rtibbles rtibbles force-pushed the cleanupsyncs_push_me_pull_you branch from 76ceb9f to 7dd81e5 Compare February 12, 2024 16:23
@rtibbles
Copy link
Member Author

Updated!

@bjester bjester self-assigned this Feb 12, 2024
Copy link
Member

@bjester bjester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prathamesh and I made sure the test coverage properly covered this, but obv improperly asserted these. So looks good to me!

@bjester bjester merged commit 5c30fde into learningequality:release-v0.16.x Feb 12, 2024
34 checks passed
@rtibbles rtibbles deleted the cleanupsyncs_push_me_pull_you branch February 12, 2024 21:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DEV: backend Python, databases, networking, filesystem... SIZE: small TODO: needs review Waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants