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.

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

[BUU] Enable admin_style_v3 for new users from 3/07/2024 #12606

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
9 changes: 8 additions & 1 deletion config/initializers/flipper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,14 @@
end
end

Flipper.register(:admins) { |actor| actor.respond_to?(:admin?) && actor.admin? }
# Groups
Flipper.register(:admins) do |actor|
actor.respond_to?(:admin?) && actor.admin?
Copy link
Member

Choose a reason for hiding this comment

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

In another PR, shall we simplify this?

Suggested change
actor.respond_to?(:admin?) && actor.admin?
actor.try(:admin?)

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea. This code is straight from the Flipper documentation. I'm guessing they chose that format to make it clear that an actor is not guaranteed to be a certain type of object.

end
Flipper.register(:new_2024_07_03) do |actor|
actor.respond_to?(:created_at?) && actor.created_at >= "2024-07-03".to_time
end

Flipper::UI.configure do |config|
config.descriptions_source = ->(_keys) do
# return has to be hash of {String key => String description}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
class ActivateAdminStyleV3ForNewUsers < ActiveRecord::Migration[7.0]
def up
Flipper.enable_group(:admin_style_v3, :new_2024_07_03)
end
end
2 changes: 1 addition & 1 deletion db/schema.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
#
# It's strongly recommended that you check this file into your version control system.

ActiveRecord::Schema[7.0].define(version: 2024_05_29_081209) do
ActiveRecord::Schema[7.0].define(version: 2024_06_25_024328) do
# These are extensions that must be enabled in order to support this database
enable_extension "pg_stat_statements"
enable_extension "plpgsql"
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
# frozen_string_literal: true

require 'spec_helper'
require_relative '../../db/migrate/20240625024328_activate_admin_style_v3_for_new_users'

RSpec.describe ActivateAdminStyleV3ForNewUsers do
it "activates new product screen for new users" do
Timecop.freeze Time.zone.parse("2024-07-03") do
user_new = create(:user)

expect {
subject.up
}.to change {
OpenFoodNetwork::FeatureToggle.enabled?(:admin_style_v3, user_new)
}.to(true)
end
end

it "doesn't activate new product screen for old users" do
Timecop.freeze Time.zone.parse("2024-07-02") do
user_old = create(:user)

expect {
subject.up
}.not_to change {
OpenFoodNetwork::FeatureToggle.enabled?(:admin_style_v3, user_old)
}
end
end
Comment on lines +19 to +29
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't quite capture the case of old users still not being activated in the future. So if the logic wasn't based on the creation date but on the current date then we wouldn't catch that.

I would have structured the test like this:

  • Create user for the past.
  • Create user for the future.
  • Test that one is disabled and the other enabled at the same time.

But I've seen the code. This is simple enough. 👍

end
Loading