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.

Sign up for GitHub

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

Jump to bottom

feat: adds TenantAwareLinkRenderStarted filter #77

Closed
wants to merge 1 commit into from
Closed

feat: adds TenantAwareLinkRenderStarted filter #77

wants to merge 1 commit into from

Conversation

JuanDavidBuitrago
Copy link
Contributor

@JuanDavidBuitrago JuanDavidBuitrago commented Mar 30, 2023 •
edited
Loading

Description:

This adds the TenantAwareLinkRenderStarted filter which receive a string context with LMS url to allowing the filter pipeline to return the information that we need to filter, in this case a tenant aware link from Studio to LMS.

An example of use is in eox-tenant custom filter step pipeline, where it takes the LMS_ROOT_URL and return only the tenant aware link in the respective site.

JIRA: N/A

Dependencies:

Merge deadline: N/A

Installation instructions:

Be sure to use edx-platform branch with changes openedx/edx-platform#32014

Testing instructions:

The change can be tested with testing instructions in eox-tenant plugin.

Reviewers:

Merge checklist:

  • All reviewers approved
  • CI build is green
  • Version bumped
  • Changelog record added
  • Documentation updated (not only docstrings)
  • Commits are squashed

Post merge:

  • Create a tag
  • Check new version is pushed to PyPI after tag-triggered build is
    finished.
  • Delete working branch (if not needed anymore)

Author concerns: N/A

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Mar 30, 2023
@openedx-webhooks
Copy link

openedx-webhooks commented Mar 30, 2023 •
edited
Loading

Thanks for the pull request, @JuanDavidBuitrago! 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:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

This is currently a draft pull request. When it is ready for our review and all tests are green, click "Ready for Review", or remove "WIP" from the title, as appropriate.

This was referenced Apr 3, 2023
Closed
Closed
@JuanDavidBuitrago JuanDavidBuitrago marked this pull request as ready for review April 3, 2023 12:53
@mphilbrick211
Copy link

Hi @mariajgrimaldi - would you mind reviewing and merging if all looks good to you? Thanks!

@mariajgrimaldi
Copy link
Member

Can you update the PR with the latest name changes? @JuanDavidBuitrago, please! FYI: I'll review this PR and the edx-platform one this week.

@JuanDavidBuitrago JuanDavidBuitrago force-pushed the JDB/add_tenant_aware_filter branch 2 times, most recently from b08f3f3 to 4138255 Compare April 18, 2023 09:14
@mariajgrimaldi mariajgrimaldi mentioned this pull request Apr 20, 2023
Closed
5 tasks
@mphilbrick211
Copy link

Hi @JuanDavidBuitrago! Would you mind taking a look at the branch conflicts?

CC @mariajgrimaldi for review.

@MaferMazu MaferMazu force-pushed the JDB/add_tenant_aware_filter branch from 4138255 to 4f3b1d6 Compare May 16, 2023 02:36
@MaferMazu MaferMazu requested a review from a team as a code owner May 16, 2023 02:36
@MaferMazu
Copy link

@mariajgrimaldi, can you help us with a review?

mariajgrimaldi
mariajgrimaldi reviewed Jun 1, 2023
View reviewed changes
openedx_filters/learning/filters.py
Custom class used to create tenant aware link render filters and its custom methods.
"""

filter_type = "org.openedx.learning.tenant_aware_link.render.started.v1"
Copy link
Member

Choose a reason for hiding this comment

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

We can't accept this being so specific. Please, refer to the openedx-events documentation for a better naming

Copy link
Member

Choose a reason for hiding this comment

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

According to your edx-platform PR, I think we could rebrand this filter by using what you're currently using it for. It could be called:
org.openedx.learning.asset.render.started.v1
And given that this filter would affect course staff only, then I'm not sure the correct subdomain is learning. Give this page a read for more info on subdomains: https://openedx.atlassian.net/wiki/spaces/AC/pages/663224968/edX+DDD+Bounded+Contexts

mariajgrimaldi
mariajgrimaldi requested changes Jun 1, 2023
View reviewed changes
Copy link
Member

@mariajgrimaldi mariajgrimaldi left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I understand these changes are useful to your initiative. The issue is that this needs to be useful as well for the community, can you make it a bit less specific? The community might not use eox-tenant or even need a tenant aware modification, how would you rebrand this modification so it's more general?

@mphilbrick211
Copy link

Hi @JuanDavidBuitrago! Just flagging Maria's comment above :)

@JuanDavidBuitrago
Copy link
Contributor Author

Hi @mphilbrick211 I'm going to work on it in this week

@mphilbrick211
Copy link

Hi @JuanDavidBuitrago! Just following-up on this :)

@mphilbrick211 mphilbrick211 added the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Jul 17, 2023
@mphilbrick211
Copy link

hi @JuanDavidBuitrago! Just checking in to see if you plan to pursue this pull request? Please let us know. Thanks!

@mariajgrimaldi
Copy link
Member

@mphilbrick211: I spoke internally with @JuanDavidBuitrago's team, and they told me they didn't have the capacity to take this on right now. I'll move this PR to draft to give them time to work on it. Thanks!

@mariajgrimaldi mariajgrimaldi marked this pull request as draft September 7, 2023 14:26
@JuanDavidBuitrago
Copy link
Contributor Author

Hi @mphilbrick211 @mariajgrimaldi, thank you for your patience.

@mphilbrick211
Copy link

Hi @JuanDavidBuitrago! I'm going to close this pull request for now. We can reopen when you're ready to pursue. Thanks!

@openedx-webhooks
Copy link

@JuanDavidBuitrago Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future.

@mphilbrick211 mphilbrick211 added the closed inactivity PR was closed because the author abandoned it label Feb 20, 2024
@mphilbrick211 mphilbrick211 removed the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Feb 20, 2024
@jignaciopm jignaciopm mentioned this pull request Jul 19, 2024
Merged
@jignaciopm jignaciopm mentioned this pull request Oct 1, 2024
Open
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mariajgrimaldi mariajgrimaldi mariajgrimaldi requested changes

Assignees
No one assigned
Labels
closed inactivity PR was closed because the author abandoned it open-source-contribution PR author is not from Axim or 2U
Projects
Contributions
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@JuanDavidBuitrago @openedx-webhooks @mphilbrick211 @mariajgrimaldi @MaferMazu