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

split user plugin #8443

Merged
merged 8 commits into from Oct 12, 2021
Merged

Conversation

indirectlylit
Copy link
Contributor

@indirectlylit indirectlylit commented Sep 17, 2021

@indirectlylit indirectlylit added this to the 0.15.0 milestone Sep 17, 2021
@indirectlylit indirectlylit marked this pull request as draft September 17, 2021 15:41
@indirectlylit indirectlylit changed the base branch from release-v0.15.x to develop September 17, 2021 15:42
@indirectlylit indirectlylit force-pushed the split-user-plugin branch 2 times, most recently from 0b7f145 to 9ca986a Compare September 29, 2021 15:03
@indirectlylit indirectlylit force-pushed the split-user-plugin branch 7 times, most recently from fe74ae9 to 41996ec Compare October 8, 2021 13:59
@indirectlylit indirectlylit marked this pull request as ready for review October 8, 2021 13:59
Copy link
Member

@rtibbles rtibbles 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 code wise - a couple of questions, but nothing blocking.

@@ -1,5 +1,6 @@
<template>
Copy link
Member

Choose a reason for hiding this comment

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

Does this need to be put into the core API? It seems like boiler plate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, it is in the core API!

https://github.com/learningequality/kolibri/pull/8443/files#diff-a2dd73b039d1c55598114bb71dc7ce6bd5fffd2e7ed8e921b218c49c53e99cc7

It's a step in the direction of this: #5719

Once 0.15 is cut, I'll apply the pattern to user_auth also

@pcenov
Copy link
Member

pcenov commented Oct 11, 2021

Hi @indirectlylit I was asked by @radinamatic to test this PR and I'm happy to report that all main workflows are functioning correctly.
At the same time I stumbled upon an issue with the setup wizard for which I'm not sure whether it's caused by any of the above changes and therefore I've reported it separately here: #8489

@indirectlylit indirectlylit force-pushed the split-user-plugin branch 3 times, most recently from 9ab4b5e to 0b95c17 Compare October 12, 2021 15:20
@indirectlylit
Copy link
Contributor Author

indirectlylit commented Oct 12, 2021

@pcenov thank you for testing!

@rtibbles I think this should be ready to go

Copy link
Member

@rtibbles rtibbles left a comment

Choose a reason for hiding this comment

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

Code looks good, QA checks out.

@rtibbles rtibbles merged commit c6d546e into learningequality:develop Oct 12, 2021
@indirectlylit indirectlylit deleted the split-user-plugin branch October 12, 2021 17:50
@rtibbles rtibbles mentioned this pull request Nov 2, 2021
9 tasks
@indirectlylit indirectlylit added P2 - normal Priority: Nice to have and removed P2 - normal Priority: Nice to have labels Nov 3, 2021
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.

None yet

3 participants