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

fix: Drop support for retiring E-Commerce Service accounts #37

Merged
merged 1 commit into from
Jul 3, 2024
Merged

fix: Drop support for retiring E-Commerce Service accounts #37

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
## Unreleased

* [Bug fix] Drop support for retiring users in the [Open edX E-Commerce Service](https://github.com/openedx/ecommerce).
Per the discussion in [Issue #36](https://github.com/hastexo/tutor-contrib-retirement/issues/36), account retirement for the E-Commerce Service "will never" work and "has never" worked, thus this is not a breaking change.

## Version 3.3.0 (2024-04-05)

* [Enhancement] Support Python 3.12.
Expand Down
9 changes: 9 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,15 @@ appropriate one:
 [which was introduced with that
 release](https://github.com/overhangio/tutor/blob/master/CHANGELOG.md#v1320-2022-04-24).


Limitations
------------

This plugin cannot be used for retiring accounts in the [Open edX E-Commerce Service](https://github.com/openedx/ecommerce).[^ecom]

[^ecom]: See [Issue #36](https://github.com/hastexo/tutor-contrib-retirement/issues/36) in this repository for a related discussion of missing functionality in E-Commerce account retirement.


Installation
------------

Expand Down
2 changes: 0 additions & 2 deletions tutorretirement/templates/retirement/build/retirement/pipeline_config/config.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,11 @@ client_secret: {{ RETIREMENT_EDX_OAUTH2_CLIENT_SECRET }}

base_urls:
lms: {{ "https" if ENABLE_HTTPS else "http" }}://{{ LMS_HOST }}
{% if ECOMMERCE_HOST is defined %}ecommerce: {{ "https" if ENABLE_HTTPS else "http" }}://{{ ECOMMERCE_HOST }}{% endif %}
Copy link

Choose a reason for hiding this comment

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

I'm not sure how important this is to the community, but if you remove this it may impact the ability to retire all of the Segment IDs. I've got doubts if anyone in the community is using ecommerce + segment + tutor, but thought I'd mention it.

tl;dr: ecommerce can maintain a separate user id just for interacting with Segment. If the retirement config sets fetch_ecommerce_segment_id to True and includes the Segment retirement step, the scripts will query Ecommerce for this id and the segment_api will retire it along with the other things that get deleted from Segment. This is what the ecommerce retirement API that we were looking at in the other issue handles.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bmtcril We maintain a community Tutor plugin here. If ecommerce is, as you say, in such a state that a reasonable Open edX community deployment shouldn't be using it at all, and the retirement pipeline for it is in such a state that there is effectively no retirement for ecommerce at all, then I don't see any impetus for a community plugin to support it. Much less support something related to the integration of that thing with a third-party service.

I'd much prefer we explicitly not support it, so that people who are brave (reckless?) enough to use ecommerce (with or without Segment) know that they're on their own as far as retirement for those services is concerned.

Copy link

Choose a reason for hiding this comment

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

Sounds good to me

{% if DISCOVERY_HOST is defined %}discovery: {{ "https" if ENABLE_HTTPS else "http" }}://{{ DISCOVERY_HOST }}{% endif %}
{% if NOTES_HOST is defined %}notes: {{ "https" if ENABLE_HTTPS else "http" }}://{{ NOTES_HOST }}{% endif %}

retirement_pipeline:
{% if FORUM_VERSION is defined %}- [ 'RETIRING_FORUMS', 'FORUMS_COMPLETE', 'LMS', 'retirement_retire_forum' ]{% endif %}
{% if ECOMMERCE_HOST is defined %}- [ 'RETIRING_ECOMMERCE', 'ECOMMERCE_COMPLETE', 'ECOMMERCE', 'retire_learner' ]{% endif %}
{% if DISCOVERY_HOST is defined %}- [ 'RETIRING_DISCOVERY', 'DISCOVERY_COMPLETE', 'DISCOVERY', 'replace_usernames' ]{% endif %}
{% if NOTES_HOST is defined %}- [ 'RETIRING_NOTES', 'NOTES_COMPLETE', 'LMS', 'retirement_retire_notes' ]{% endif %}
- [ 'RETIRING_EMAIL_LISTS', 'EMAIL_LISTS_COMPLETE', 'LMS', 'retirement_retire_mailings' ]
Expand Down
Loading