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

Conversation

dacook
Copy link
Member

@dacook dacook commented Jun 25, 2024

What? Why?

What should we test?

This is hard to test on staging, so I have done a dev test.

1. Migration

rake db:migrate

Screenshot 2024-06-25 at 1 13 51 pm

2. User created before 3rd July:

Screenshot 2024-06-25 at 1 15 04 pm

3. User created on 3rd July:

u = Spree::User.find 4
u.created_at = Time.zone.parse("2024-07-03")
u.save!

Screenshot 2024-06-25 at 1 15 00 pm

Release notes

Changelog Category (reviewers may add a label for the release notes):

  • User facing changes
  • API changes (V0, V1, DFC or Webhook)
  • Technical changes only
  • Feature toggled

The title of the pull request will be included in the release notes.

Dependencies

It's not strictly a dependency, but now that we are phasing the new design in, the test suite should be using the new design asap:

Documentation updates

Uh.. yeah everything? 😕
I think the old screenshots will be ok, but will look dated and it would be good to refresh them sometime.

More importantly, the Bulk Products screen will be different. I could only find one page in the guide for this:

@dacook dacook added the user facing changes Thes pull requests affect the user experience label Jun 25, 2024
@dacook dacook self-assigned this Jun 25, 2024
Copy link
Member

@mkllnk mkllnk left a comment

Choose a reason for hiding this comment

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

Great!

I don't think that this will need any more testing.

Comment on lines +19 to +29
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
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. 👍

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.

Copy link
Collaborator

@rioug rioug left a comment

Choose a reason for hiding this comment

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

Nice one 👍

@rioug rioug merged commit cb98f2f into openfoodfoundation:master Jun 25, 2024
52 checks passed
@rioug
Copy link
Collaborator

rioug commented Jun 25, 2024

Do we need to follow up with someone for the documentation update ?

@dacook
Copy link
Member Author

dacook commented Jun 25, 2024

All good, Rachel confirms this is taken care of by instance managers (https://openfoodnetwork.slack.com/archives/C01CXQNJ1J6/p1719286230825269?thread_ts=1719239630.576049&cid=C01CXQNJ1J6)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
user facing changes Thes pull requests affect the user experience
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Allow to turn the feature toggle on for all new users
3 participants