Namespace plan check follow-up - facilitate feature addition

As a follow-up for https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/1961, we should make the following improvements:

One should not have to change more than one place to map a new feature for a plan

Today, to map a new feature, one should:

• Create a feature constant
• Add feature: NEW_FEATURE on FEATURE_CODES Hash - that is on License
• Add NEW_FEATURE => 1 on EE*_FEATURES Hash - also on License (to mimic the legacy pattern we used to fetch on License#add_ons)

Ultimately, we should be able to map those features in a way that only a new Symbol addition on a plan list would be enough.

Follow can? method pattern

One should be able to call can?(user, :feature, project) (wrapping Project#feature_available?) in order to stay consistent with the rest of the permission checking.

cc @DouweM

changed the description

@to1ne You may want to play a bit with this as well while you're on https://gitlab.com/gitlab-org/gitlab-ee/issues/2335.

assigned to @to1ne

mentioned in merge request !1916 (merged)

mentioned in issue #2761 (closed)

@nick.thomas @reprazent now #2335 (closed) is nearly finished, where do you think we can improve?

Adding the MY_AWESOME_FEATURE = 'GitLab_AwesomeFeature'.freeze and adding it to FEATURE_CODES feels like a redundant action, but it's not really bothering me.

Having a globally scoped licensed_feature_available? (following can? pattern), would help a bit. But isn't it a good thing we now explicitly ask the Project or the Namespace if a feature is available? I feel adding a global method would have too much magic going on once implemented?

changed milestone to %9.4

@to1ne @oswaldo does this really need to be a confidential issue?

I'm a fan of avoiding magic global methods. I don't like a global can? and I wouldn't like a global licensed_feature_available? either.

I think License does need a lot of cleaning up. Right now it's full of redundant information and it's hard to understand.

Perhaps we can use something a little DSL-y? Take the following as a starting-point.

class License
  ees_features do
    # automatically generates the feature name: 'GitLab_BurndownCharts'
     burndown_charts early_adopter: true
  end

  eep_features do
    # can't generate the feature name string automatically
    auditor_user early_adopter: true, code: 'GitLab_Auditor_User'
  end
end

I'm a fan of avoiding magic global methods. I don't like a global can? and I wouldn't like a global licensed_feature_available? either.

Seconded. Calling the .feature_available? on an object makes it clear where to look to see what's going on. In the case of Project & Group it provides the correct context for the feature check which looks nicer than passing it as a parameter.

Perhaps we can use something a little DSL-y? Take the following as a starting-point.

It sure looks nice. And since we'd only need to add a single line for a feature, it would be only 1 conflict at a time instead of the current 3 or 4 .

I would like to see which plan is extended though, something like:

eep_features extends: ees_fetures do
  auditor_user early_adopter: true, code: 'GitLab_Auditor_User'
end

I don't think this issue still needs to be confidential.

made the issue visible to everyone

I think we could define Early Adapter like :early_adopter instead of early_adopter: true.

Also, maybe it's an opportunity to define dependencies between features, like @nick.thomas mentioned before Burndown Charts depend on Issue Weights. By defining this somehow we might avoid future mistakes?

  eep_features do
    # can't generate the feature name string automatically
    auditor_user early_adopter: true, code: 'GitLab_Auditor_User'
  end

@nick.thomas In which cases can't the feature name string be generated automatically?

Note: Defining the DSL like this would only allows us to create plans that extend each other. But I don't expect this to be a problem in the somewhat near future.

@nick.thomas In which cases can't the feature name string be generated automatically?

@to1ne we need to choose a convention for generating the string from the code, and it's probably a mistake to change any of the existing strings.

If :burndown_carts becomes GitLab_BurndownCharts and :auditor_user becomes GitLab_Auditor_User, then one or the other will need to be manually specified.

it's probably a mistake to change any of the existing strings.

@nick.thomas I am not sure why this is the case??

Also, if we like to change some of the strings, we might want to do it now, just before the %9.4 merge window is closed (leaving those added in %9.3 alone).

If they appear in any licenses as addons, changing the string will break the license. I guess this isn't a problem for stuff added between %9.3 and %9.4 though.

@nick.thomas Wouldn't now be a good time to make them uniform then?

I haven't been using the Gitlab_ prefix

mentioned in merge request !2360 (merged)

OK, I've created https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/2360 to rationalise the existing codes. If there's MRs left to be merged, perhaps ensure they follow the same pattern?

@nick.thomas Thanks ! I've updated my open MRs.

We should also add developer documentation for this, to answer questions like: 'how do I add a license check for my feature?'!

OK, we can't change GitLab_Auditor_User, so we'll need a mechanism to special-case it: https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/2360#note_34345087

I think we could define Early Adapter like :early_adopter instead of early_adopter: true.

Here's a thought. If we get our implementation to return the feature code symbol from the defining method call, it could look like:

early_adopter auditor_user code: 'GitLab_Auditor_User'

Maybe this is all getting a bit too magic though. We could keep early_adopter separated out:

auditor_user code: 'GitLab_Auditor_User'
# ...
early_adopter :auditor_user, :deploy_board, ....

Thanks for the suggestions and https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/2360 @nick.thomas!

Food for thought:

Couldn't we simplify things and avoid using a DSL by just declaring this on a yml file? We could eager-load this on the GitLab instance load to avoid disk-accesses on runtime.

@oswaldo that sounds like a good idea. We could have a yaml file like:

---
ees_features:
  auditor_user: GitLab_Auditor_User
eep_features:
  deploy_board:
eeu_features:
  ...
early_adopter:
  - auditor_user
  ...

The one downside I see is that it then almost begs to be modified by unscrupulous users. Obviously, they could modify app/models/license.rb as well, but this somehow feels more likely to happen.

@DouweM So it seems we only like to rewrite how features & plans are defined in app/models/license.rb. Do you like us to implement @nick.thomas's yaml proposal above? Or some other proposal? Or just leave it as is?

@to1ne @nick.thomas I don't mind keeping it in license.rb, but I'm pretty sure we should be able to go from:

AUDIT_EVENTS_FEATURE = 'GitLab_AuditEvents'.freeze
FEATURE_CODES = {
  audit_events: AUDIT_EVENTS_FEATURE
}
EES_FEATURES = [
  { AUDIT_EVENTS_FEATURE => 1 }
]

to:

EES_FEATURES = [
  :audit_events
]

That makes adding a new feature a lot less cumbersome.

We only need to keep a mapping of legacy addon names to feature names ("GitLab_FooBar" => :foo_bar) for the sake of older licenses that include addons in their payload.

We could implement a DSL or yml, but in this case I don't see an obvious benefit over plain old Ruby.

@oswaldo Any thoughts?

changed milestone to %10.0

Agree with https://gitlab.com/gitlab-org/gitlab-ee/issues/2466#note_36751957 @DouweM. So in License.features_by_plan we might have something like (not tested):

def self.features_for_plan(plan)
  FEATURES_BY_PLAN.fetch(plan, []).map do |feature_sym|
    if LEGACY_LICENSE_FEATURES.key?(feature_sym)
      { LEGACY_LICENSE_FEATURES[feature_sym] => 1 }
    else
      { feature_sym.to_s => 1 }
    end
  end.reduce({}, :merge)
end

Note that the only real code that we need to properly map are the legacy ones (these have the GitLab_ thing - it's persisted like this on the license, so we need to keep the exact same name):

LEGACY_LICENSE_FEATURES = {
  auditor_user: AUDITOR_USER_FEATURE,
  deploy_board: DEPLOY_BOARD_FEATURE,
  service_desk: SERVICE_DESK_FEATURE,
  file_locks: FILE_LOCKS_FEATURE,
  geo: GEO_FEATURE
}

@oswaldo I think it could be easier :) Something like:

EES_FEATURES = [
  :issue_weights
]

EEP_FEATURES = [
  *EES_FEATURES,
  :file_locks
]

FEATURES_BY_PLAN = {
  STARTER_PLAN       => EES_FEATURES,
  PREMIUM_PLAN       => EEP_FEATURES
}.freeze

FEATURES_FOR_ADD_ONS = {
  'GitLab_IssueWeights' => :issue_weights
}

def self.features_for_plan(plan)
  FEATURES_BY_PLAN.fetch(plan, [])
end

def features_from_add_ons
  add_ons = restricted_attr(:add_ons, {})

  add_ons.map { |name, count| FEATURES_FOR_ADD_ONS[name] if count > 0 }.compact
end

def features
  @features ||= (self.class.features_for_plan(plan) + features_from_add_ons).to_set
end

def feature_available?(feature)
  features.include?(feature)
end

changed milestone to %10.1

assigned to @DouweM and unassigned @to1ne

mentioned in merge request !2943 (merged)

closed via merge request !2943 (merged)

closed via commit c653ae39

mentioned in commit c653ae39