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

MVP for new feature flags admin console #2080

Open
wants to merge 38 commits into
base: main
Choose a base branch
from

Conversation

EmmaFrith
Copy link
Contributor

Context

Ticket: https://dfedigital.atlassian.net/browse/CPDNPQ-2180

The existing feature flags section is difficult to use and understand for people who aren't developers. We're rebuilding the features section so that:

  • it's in GOV.UK style
  • it's in keeping with the rest of the admin console
  • features have a description so that people know what they do

We want to do this so that relevant members of the NPQ registration team feel confident turning features on and off when needed.

Changes proposed in this pull request

We've added a new MVP feature flags section to the legacy admin console.

This currently sits alongside the existing feature flags section. This is so that we still have all the existing functionality as we build out the new MVP.

The new section includes:

  • an index page showing all the features that are in use and not in use (not in use means that they are in the database, but we haven't set them out as a feature that should be used - see the FEATURE_FLAG_KEYS array for those that should be used)
  • a show page for each feature that is in use, where you can change the state of the feature and see how many actors there are for the feature

Things we could do next:

  • better error handling, in line with GOV.UK design system, on the form to change the feature flag state
  • create tests
  • add functionality to delete features that aren't in use
  • look at whether we need CRUD functionality on the actors section - i.e. deleting an actor
  • add descriptions to all the feature flags - currently we only have descriptions for some of them
  • make descriptions essential when creating a feature flag

EmmaFrith and others added 29 commits November 28, 2024 14:33
@EmmaFrith EmmaFrith requested a review from a team as a code owner December 16, 2024 16:12
@EmmaFrith EmmaFrith changed the title Scoping new admin page feature flags MVP for new feature flags admin console Dec 16, 2024
Copy link

@EmmaFrith EmmaFrith self-assigned this Dec 16, 2024
Copy link
Contributor

@jebw jebw left a comment

Choose a reason for hiding this comment

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

Hey, this looks really good and is a really solid base to build off - it all works great.

There's a four items I'm keen to address - we can work through these this week

  • its super minor but there are linting tools we set your computer with (kinda like grammar checkers for developers)
  • permissions checks - this checks for admin rights but would be good to extend this to check for super admins instead
  • we could do with filtering the features users are allowed to change to just the ones listed in the FeatureFlags class
  • it would be great to add a feature spec to automate checking this works as intended

app/views/admin/features/show.html.erb Outdated Show resolved Hide resolved
app/services/feature.rb Outdated Show resolved Hide resolved
app/views/admin/features/index.html.erb Show resolved Hide resolved
end

def show
@feature = params[:id]
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be good to check the feature exists and is one in active use, particularly since the user supplied name is being reflected back to them in the flash message

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh that's such a good point! I hadn't noticed that you can put anything anything at all into the url and you get a show page for it. So would it be something like this?

def show
  @feature = params[:id]
  if Feature::FEATURE_FLAG_KEYS.include?(@feature)
    @users = User.with_feature_flag_enabled(@feature)
  else
    redirect_back fallback_location: admin_features_path
  end
end    

@@ -0,0 +1,29 @@
class Admin::FeaturesController < AdminController
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is going to need something more on the permissions checks - we can chat this through

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants