Unify feature check for organization/project #8920

stsewd · 2022-02-14T23:54:58Z

We are overriding classes and skipping tests con .com just because on .com we require a subscription for some features.
Now that we have subs on .org we can share more code between both sites. I have added a new setting to check if features should always be present or if we should check for a subscription.

Refs #9313

stsewd · 2022-02-16T20:29:12Z

readthedocs/projects/querysets.py

+                project,
+                type=PlanFeature.TYPE_CONCURRENT_BUILDS,
+                default=settings.RTD_MAX_CONCURRENT_BUILDS,
+            )


we had an override just to check for the value from the plan on .com.

Shouldn't this method contain all the logic? I mean, also check for project.max_concurrent_builds or max_concurrent_organization as well?

Otherwise, it's only being called for the default value, but it won't affect the project's value.

It seems my suggestion is already applied, right?

humitos

This looks good as a start. However, I think it needs to encapsulate more logic in the .get_feature_value() so we can trust 100% on it instead of mixing it with some extra logic in other places (e.g. max concurrent builds)

I'd like to do PlanFeature.objects.get_feature_value(project, type=PlanFeature.TYPE_CONCURRENT_BUILDS, default=settings.RTD_MAX_CONCURRENT_BUILDS) and don't worry if the project is under an organization or not and if it has a particular max concurrent defined at the project level. I'd like to rely on the .get_feature_value() to handle all that logic for me.

BTW, it would have been good to have a better description in the PR to understand what this code should do to reduce guessing.

readthedocs/settings/base.py

readthedocs/organizations/views/private.py

humitos · 2022-03-07T10:51:07Z

readthedocs/projects/querysets.py

+                project,
+                type=PlanFeature.TYPE_CONCURRENT_BUILDS,
+                default=settings.RTD_MAX_CONCURRENT_BUILDS,
+            )


Shouldn't this method contain all the logic? I mean, also check for project.max_concurrent_builds or max_concurrent_organization as well?

Otherwise, it's only being called for the default value, but it won't affect the project's value.

readthedocs/projects/views/private.py

humitos · 2022-03-07T11:02:23Z

readthedocs/subscriptions/managers.py

+        if not settings.RTD_ALL_FEATURES_ENABLED:
+            feature = self.get_feature(obj, type)
+            if feature:
+                return feature.value
+        return default


Here is where I think it needs to encapsulate more logic, so we can rely 100% on it without mixing the .get_feature_value with other logic in the code.

stsewd · 2022-03-07T23:31:47Z

I think it needs to encapsulate more logic in the .get_feature_value() so we can trust 100% on it instead of mixing it with some extra logic in other places (e.g. max concurrent builds)

Not really sure about that, concurrent build is the only one, and is very specific about the attributes and order that should be checked. It makes more sense to have that on project model class.

humitos · 2022-03-08T09:09:56Z

Not really sure about that, concurrent build is the only one, and is very specific about the attributes and order that should be checked.

Yeah, even if it's the only one now, I think it could be better to handle each special case we may have in the future inside the .get_feature_value() so all logic related to "getting the real value for a feature" is in the same place instead of in multiple parts of the codebase --producing, maybe, different logic/results as it has to be duplicated.

stsewd · 2022-03-09T01:01:58Z

all logic related to "getting the real value for a feature" is in the same place instead of in multiple parts of the codebase --producing, maybe, different logic/results as it has to be duplicated.

I'm not really convince about having that special case there, the logic is already encapsulated in the project queryset

readthedocs.org/readthedocs/projects/querysets.py

Line 92 in 60f0016

def max_concurrent_builds(self, project):

.

If someone else thinks that's fine, I can change it.

humitos · 2022-03-09T08:45:54Z

the logic is already encapsulated in the project queryset

That's my point here. For all the PlanFeature(s) it's enough to do PlanFeature.objects.get_feature_value but for this feature in particular, max concurrent builds, you have to do something different: call the queryset. I'm saying that it would be good to put the logic of the queryset inside the .get_feature_value so there is no difference among any of the plan features to get the final feature value.

stale · 2022-05-02T08:23:20Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stsewd · 2022-11-23T16:56:29Z

I have changed this slightly, instead of having a setting to enable all features, now we have a setting to enable a set of features, this is for users that don't have a subscription (.org), on .com this will {} to rely entirely on the features from the subscription.

I'd like to merge this, so we can continue with the djstripe migration, this is useful since we would have only one place where to change the queryset/lookup logic. This also removes a lot of overrides on .com (and probably we can remove more!).

The only blocker before was a disagreement on #8920 (comment), I still don't think we should merge that logic there (we only want to know the value of the feature, not the value of the org/project), and the organization.max_concurrent_builds probably should be removed and rely only on the value from the subscription (I think we added that in order to sell more builders, but we now can have that value in the sub itself), and we already have that logic encapsulated in the queryset #8920 (comment).

ericholscher · 2023-01-18T23:47:03Z

Looks like there's an update here, if you remember much from the past to review it @humitos

humitos

Looks good. I left some minor details to be considered and make the code more readable, basically.

humitos · 2023-01-30T17:21:54Z

readthedocs/subscriptions/constants.py

+TYPE_CNAME = "cname"
+TYPE_CDN = "cdn"
+TYPE_SSL = "ssl"
+TYPE_SUPPORT = "support"
+
+TYPE_PRIVATE_DOCS = "private_docs"
+TYPE_EMBED_API = "embed_api"
+TYPE_SEARCH_ANALYTICS = "search_analytics"
+TYPE_PAGEVIEW_ANALYTICS = "pageviews_analytics"
+TYPE_CONCURRENT_BUILDS = "concurrent_builds"
+TYPE_SSO = "sso"
+TYPE_CUSTOM_URL = "urls"
+TYPE_AUDIT_LOGS = "audit-logs"
+TYPE_AUDIT_PAGEVIEWS = "audit-pageviews"


Is there any reason why these are prefixed with TYPE_ instead of FEATURE_? TYPE_EMBED_API does not make too much sense to me, but FEATURE_EMBED_API does.

In places like this one, it will be more readable: https://github.com/readthedocs/readthedocs.org/pull/8920/files#diff-e7f36d6ba29b1ae0d1c6e12781ba3f54dc4cb434cf07057f73e2f5d57b545ae9R166

This is also why I've preferred model constants are defined on the models themselves. The added context and readability of Feature.TYPE_EMBED_API or Feature.EMBED_API is quite nicer than a list of constants.

This is a place where I'd love to use enums instead p:

humitos · 2023-01-30T17:25:50Z

readthedocs/projects/views/private.py

    def _is_enabled(self, project):
        """Should we show search analytics for this project?"""
-        return True
-
-
-class SearchAnalytics(SettingsOverrideObject):
-    _default_class = SearchAnalyticsBase
+        return PlanFeature.objects.has_feature(
+            project,
+            type=self.feature_type,
+        )


This method should go in a mixin since it's shared among multiple classes.

humitos · 2023-01-30T17:26:53Z

readthedocs/organizations/views/private.py

+    def _is_feature_enabled(self, organization):
+        return PlanFeature.objects.has_feature(
+            organization,
+            type=self.feature_type,
+        )


This is the same method as _is_enabled. However, it uses organization= instead of project= and it's named differently. We should decide whether naming it _is_feature_enabled or _is_enabled to keep consistency.

humitos · 2023-01-30T17:28:05Z

readthedocs/projects/querysets.py

+                project,
+                type=PlanFeature.TYPE_CONCURRENT_BUILDS,
+                default=settings.RTD_MAX_CONCURRENT_BUILDS,
+            )


It seems my suggestion is already applied, right?

ericholscher · 2023-02-14T19:07:11Z

@stsewd Is this ready to get merged as well?

stsewd · 2023-02-14T21:13:06Z

@stsewd Is this ready to get merged as well?

Mostly I guess, need to update the PR, maybe revising using enums at #8920 (comment).

humitos · 2023-03-07T10:17:02Z

@stsewd It would be good to move forward with this PR since the work is already done and reviewed. Can you update it to be in a meargeable state?

stsewd force-pushed the unify-feature-check branch from ca94e89 to e489f0c Compare February 16, 2022 18:03

Unify feature check for organization/project

f29401f

stsewd force-pushed the unify-feature-check branch from e489f0c to f29401f Compare February 16, 2022 18:48

stsewd added 3 commits February 16, 2022 14:07

Explicit tests

8e339c0

swap

b58e282

Another test

274a377

stsewd commented Feb 16, 2022

View reviewed changes

stsewd marked this pull request as ready for review February 16, 2022 20:30

stsewd requested a review from a team as a code owner February 16, 2022 20:30

humitos requested changes Mar 7, 2022

View reviewed changes

stsewd added 3 commits March 8, 2022 19:48

Updates from review

277a513

Merge branch 'master' into unify-feature-check

5ad812e

Black

60f0016

stale bot added the Status: stale Issue will be considered inactive soon label May 2, 2022

stsewd removed the Status: stale Issue will be considered inactive soon label May 3, 2022

stsewd added 2 commits November 22, 2022 13:24

Merge branch 'main' into unify-feature-check

7dae9b4

Merge branch 'main' into unify-feature-check

248b567

stsewd marked this pull request as draft November 22, 2022 18:31

stsewd added 7 commits November 22, 2022 17:07

Test with alternate implementation

8911ded

Linter

330e1f6

Explicit features

cfa0c66

One more

bf4199b

More explicit tests

33c4a30

Linter

d011326

Update docstrings

6d9f144

Update tests

9d23f79

stsewd marked this pull request as ready for review November 23, 2022 16:56

auto-assign bot assigned stsewd Nov 23, 2022

stsewd requested a review from humitos November 23, 2022 16:56

Merge branch 'main' into unify-feature-check

444c65e

stsewd mentioned this pull request Jan 18, 2023

Subscriptions: use djstripe for Plan and PlanFeature models #9313

Closed

4 tasks

humitos approved these changes Jan 30, 2023

View reviewed changes

stsewd added 6 commits April 4, 2023 15:52

Merge branch 'main' into unify-feature-check

ff12df7

Black

bff4ab4

CND is available in all plans now

652c62c

Don't hit the DB if subscriptions aren't allowed

39581af

Check for custom domains

21473bc

Be explicit in tests

65b33df

stsewd merged commit 6edc096 into main Apr 5, 2023
7 checks passed

stsewd deleted the unify-feature-check branch April 5, 2023 18:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unify feature check for organization/project #8920

Unify feature check for organization/project #8920

stsewd commented Feb 14, 2022 •

edited by ericholscher

stsewd Feb 16, 2022

humitos Mar 7, 2022

humitos Jan 30, 2023

humitos left a comment •

edited

humitos Mar 7, 2022

humitos Mar 7, 2022

stsewd commented Mar 7, 2022

humitos commented Mar 8, 2022

stsewd commented Mar 9, 2022

humitos commented Mar 9, 2022

stale bot commented May 2, 2022

stsewd commented Nov 23, 2022

ericholscher commented Jan 18, 2023

humitos left a comment

humitos Jan 30, 2023

agjohnson Jan 30, 2023

stsewd Feb 2, 2023

humitos Jan 30, 2023

humitos Jan 30, 2023

humitos Jan 30, 2023

ericholscher commented Feb 14, 2023

stsewd commented Feb 14, 2023

humitos commented Mar 7, 2023

Unify feature check for organization/project #8920

Unify feature check for organization/project #8920

Conversation

stsewd commented Feb 14, 2022 • edited by ericholscher

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

humitos left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stsewd commented Mar 7, 2022

humitos commented Mar 8, 2022

stsewd commented Mar 9, 2022

humitos commented Mar 9, 2022

stale bot commented May 2, 2022

stsewd commented Nov 23, 2022

ericholscher commented Jan 18, 2023

humitos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericholscher commented Feb 14, 2023

stsewd commented Feb 14, 2023

humitos commented Mar 7, 2023

stsewd commented Feb 14, 2022 •

edited by ericholscher

humitos left a comment •

edited