-
Notifications
You must be signed in to change notification settings - Fork 17
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
feat: add filter before user setttings context is rendered #46
feat: add filter before user setttings context is rendered #46
Conversation
Thanks for the pull request, @Henrrypg! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
Hello! To contribute to this repo you'll have to sign a CLA first. cc @felipemontoya |
6a75bde
to
510f87e
Compare
9050e18
to
034f2b4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello! Thanks for your contribution. Can you please fix the CI checks?
131f0bc
to
9133bc3
Compare
Thank you @mariajgrimaldi, can you check with this changes? |
9133bc3
to
42eaef8
Compare
42eaef8
to
1b72ef2
Compare
Hi @Henrrypg - just checking to see if the requested changes were made and/or this pull request is ready for review. Thanks! |
@mariajgrimaldi I just made your suggested minor changes and some changes with exceptions in favor of your suggestion |
I'll check on this again tomorrow! @Henrrypg: can you rebase with main? Thanks! |
58a7551
to
5d17a34
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good! Thanks for your patience and for following my suggestions!
@Henrrypg: there are some issues with unittests. Can you help me fix them? |
@mariajgrimaldi Of course, i'll work on this. Thank you! |
@mariajgrimaldi @mphilbrick211 can you please re run tests? :D |
@Henrrypg: sorry to bother, can you add a changelog record for the filter? so we can release the library after emerging. |
Hi @mariajgrimaldi! When this is good-to-go, do you mind merging? |
Hi @mphilbrick211 @mariajgrimaldi, i just commited the changelog and bump version |
@Henrrypg 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
Description: This PR is to add one filter:
AccountSettingsRenderStarted
filter which passes the account settings context before is rendered.JIRA:
No direct openedx related ticket is available. Related tickets that add context to this change are:
Dependencies:
Merge deadline: List merge deadline (if any)
Installation instructions: List any non-trivial installation
instructions.
Testing instructions:
The example application using this filter is implemented in edunext-platoform and its PR contains the testing instructions.
Reviewers:
Merge checklist:
Post merge:
finished.
Author concerns: None