Naming Convention to Avoid Overriding Predicate Methods

Everyone can contribute. Help move this issue forward while earning points, leveling up and collecting rewards.

• Close this issue

Summary

We have a number of places where we override the predicate method of a boolean field generated by rails. This could be surprising because one would expect the autogenerated predicate method to return what is in the database for the object based on convention.

Some examples are merge_trains_enabled? https://gitlab.com/gitlab-org/gitlab/-/blob/master/ee/app/models/ee/project_ci_cd_setting.rb#L16 or forward_deployment_enabled?/. This happens because we may need to take into account feature flags or another setting object to know how the application should behave.

This proposal is to have a naming convention for these scenarios where the setting in the database has a different name than the setting used within the business logic that avoids overriding the generated predicate.

Proposal

  # bad: ActiveRecord predicate method override
  def merge_trains_enabled?
    super &&     
    merge_pipelines_enabled? &&
    project.feature_available?(:merge_trains)
  end

  # good: database column predicate method is not over-ridden
  def merge_trains_available_and_enabled?
    merge_trains_enabled? && 
    merge_pipelines_enabled? &&
    project.feature_available?(:merge_trains)
  end

This came up during code review here:

!50889 (comment 490112562)

Improvements

Code is less surprising, because the rails conventions are respected.

Acceptance Criteria

• Get feedback/approval from the backend maintainers
• Document the preferred convention
• Change at least 1 CI feature to use the new convention (merge_trains_enabled?) suggested
• Create new issues for the other CI features and label them as good for onboarding and community contributors