refactor: remove ENABLE_TAXES_DISPLAY feature flag #3354
Conversation
|
I don't think there's any reason to keep this feature flag. The feature has been deployed, and there's no reason why we would turn it off (at least not in RC and production deployments).
|
|
@cachob I am going to remove the feature flag ENABLE_TAXES_DISPLAY as per Peter's suggestion. |
@pdpinch could you take a look at the other subtasks in https://github.com/mitodl/hq/issues/5825 and let us know what other flags are not needed? |
See https://github.com/mitodl/hq/issues/5825#issuecomment-2436216514 If the flag is currently True in production, we should plan to remove it. I would only consider keeping these flags if they are significantly valuable for developers. Otherwise, they are just tech debt. |
Wow. Sorry, I missed that. |
asadali145
changed the title
|
@arslanashraf7 This is good to review. I have removed the usage of ENABLE_TAXES_DISPLAY. |
There was a problem hiding this comment.
Kept it intentionally for the features that we will move to posthog. It is similar to mitxonline https://github.com/mitodl/mitxonline/blob/main/main/features.py
There was a problem hiding this comment.
I see. But it doesn't make sense to add it to this PR as this is out of context for this PR. We should add this with a PR that migrates the first feature flag.
| @@ -494,7 +495,7 @@ def test_get_js_settings(settings, rf, user): | |||
| "digital_credentials_supported_runs": settings.DIGITAL_CREDENTIALS_SUPPORTED_RUNS, | |||
| "course_dropdown": settings.FEATURES.get("COURSE_DROPDOWN", False), | |||
| "webinars": settings.FEATURES.get("WEBINARS", False), | |||
| "enable_taxes_display": settings.FEATURES.get("ENABLE_TAXES_DISPLAY", False), | |||
| "enable_taxes_display": display_taxes(request), | |||
There was a problem hiding this comment.
nit: enable_taxes_display gives a sense of a feature flag somewhere, for more readability we should change its name as well.
NOTE: This will require changes in various other places as this name is used in multiple locations. The logic isn't complex so this might be an easy name change.
As for the method name changes, We can go for has_tax_rate which is all we are going to check from now onwards.
There was a problem hiding this comment.
Yeah, I saw that and avoided extra changes and focused on the removal only. I can make these changes if you want.
There was a problem hiding this comment.
This is already a nit so It's up to you. I looked at the usages of this name and they looked straight forward and there were not too many. This is mostly for readability.
| @@ -165,13 +165,10 @@ def display_taxes(request): | |||
| request(HttpRequest): Request object | |||
There was a problem hiding this comment.
I think we should write a test for this method if not already. Since we are not mocking it in different tests and if we add additional conditions in the future the mock would always stay true/false.
There was a problem hiding this comment.
There is already a test for this in ecommerce/api_test.py named test_display_taxes.
What are the relevant tickets?
Closes https://github.com/mitodl/hq/issues/6369
Description (What does it do?)
Removes ENABLE_TAXES_DISPLAY feature flag.
How can this be tested?