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

Overhaul the admin and support for required condition #35

Merged
merged 7 commits into from
Jun 24, 2019

Conversation

willbarton
Copy link
Member

@willbarton willbarton commented Mar 20, 2019

This PR makes significant changes to the Wagtail-Flags admin to:

  • support Django-Flags's recently introduced "required" conditions,
  • make comprehending the conditions for a large number of flags comprehensible by moving them to subpages, and
  • hide "boolean" conditions behind "enable"/"disable" buttons to make their intent clearer. "booleans" are intended to be simple on/off switches, so this tries to express that in the UI.

This looks like:

New admin workflow

The great-big-list-of-all-flags-and-conditions is still available behind a WAGTAILFLAGS_ADMIN_BIG_LIST feature flag. If that flag is enabled, the new individual flag UI on the subpages will instead be displayed on the main flags page.

My current plan is, once this is reviewed, to release a beta, and we can get feedback on it in cf.gov. Once we decide on a direction between subpages and great-big-list, then we'll remove the WAGTAILFLAGS_ADMIN_BIG_LIST feature flag and release 4.1.

@Scotchester
Copy link
Contributor

Eeeeee! Exciting stuff! I will try to review today, but one initial point of feedback: I think it would be nice to still show all conditions on a flag in the main list of flags.

@willbarton
Copy link
Member Author

I think it would be nice to still show all conditions on a flag in the main list of flags.

I find that list overwhelming, personally, with all the flags in cfgov-refresh. What if it were hidden behind a disclosure, so it's still a click away, but not a page-load away?

@Scotchester
Copy link
Contributor

I think it would be nice to still show all conditions on a flag in the main list of flags.

I find that list overwhelming, personally, with all the flags in cfgov-refresh. What if it were hidden behind a disclosure, so it's still a click away, but not a page-load away?

That would be a nice middle ground.

@willbarton
Copy link
Member Author

@Scotchester I cannot find a way to marry the UI I have here with a disclosure that exposes all the conditions on the same page. I think I'd personally make a judgement that it's better to dig into what the conditions for a flag might be, but I'm going to hold off, because I think I want to move some of this functionality into Django-Flags, and that might inform a different approach here.

This PR is on hold until then.

This changes the Wagtail Flags UI to be a multi-level way to browse flags and their conditions. It allows you to edit existing conditions, delete conditions, and add new ones to each flag, as well as to add a flag itself in the UI.

It also hides "boolean" conditions behind an "Enable"/"Disable" set of buttons so that their purpose is clearer.
The requirements should match what we're testing against.
@willbarton
Copy link
Member Author

@Scotchester I think the best alternative is going back to having everything on one page:

image

@Scotchester
Copy link
Contributor

That UI makes for a pretty crazy long page on cf.gov... but maybe that gives us some impetus to continue cleaning up our outdated flags :)

I'm good either way. I leave it to your judgment.

@willbarton
Copy link
Member Author

@Scotchester I'll push up the one-big-list behind a... feature flag! And then we can get folks's opinions. I think I'm most comfortable leaving the default as a multiple-page drill drown.

This change will restore the one-big-list-of-all-flags in the admin if the WAGTAILFLAGS_ADMIN_BIG_LIST flag is enabled.
@@ -0,0 +1,63 @@
{% load i18n %}
{% load wagtailflags_admin %}
{% load i18n feature_flags flags_debug wagtailflags_admin %}
Copy link
Member

Choose a reason for hiding this comment

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

Does this load wagtailflags_admin twice?

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed so.

Copy link
Member

@higs4281 higs4281 left a comment

Choose a reason for hiding this comment

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

This is a wonderful improvement, especially for the common use case – a single on/off switch.

Because the on/off usage is so common, I prefer the simplified list option to cut down visual clutter.

I just had one minor question.

@schbetsy
Copy link

The plain language describing flag conditions is great. I'm into it. This is the only one that had me really confused:
Screen Shot 2019-06-21 at 4 10 54 PM

"disabled when all required conditions are met" == enabled when the conditions aren't met? I didn't get to drill into the logic to figure it out

@willbarton
Copy link
Member Author

willbarton commented Jun 21, 2019

@schbetsy Ah yes. That's meant to indicate that the flag is disabled even when all required conditions are met. COMPLAINT_INTAKE_TECHNICAL_ISSUES has an explicit boolean False condition that means that even if the required conditions are met, it'll still be disabled.

Does that make sense? Is there a better way to indicate that?

@schbetsy
Copy link

Ah, that makes sense. Maybe "disabled for all requests, regardless of whether conditions are met"?

@willbarton
Copy link
Member Author

Excellent suggestion. That'll have to be in Django-Flags, actually. I'll PR and tag you there.

@willbarton
Copy link
Member Author

willbarton commented Jun 24, 2019

PR is open at cfpb/django-flags#45

That'll be in Django-Flags 4.2.1, which I don't think is worth pinning explicitly in Wagtail-Flags (I'm going to leave the version pinning at >=4.2,<5.0), so I'm going to go ahead and merge this and release the beta so we can get some feedback in cfgov-refresh.

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.

None yet

4 participants