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

remove use of FEATURE_FLAGS_NEW_SUPPLIER_FLOW, bump digitalmarketplace-utils to v43.0.0 #129

Merged
merged 2 commits into from
Aug 24, 2018

Conversation

risicle
Copy link
Contributor

@risicle risicle commented Aug 24, 2018

See https://trello.com/c/ZH9trgjH/419-snyk-alert-for-flask-010x-vulnerability

Crown-Commercial-Service/digitalmarketplace-utils#447

This one required a bit of fiddling to remove its use of feature flags. From the changeset comment:

remove use of FEATURE_FLAGS_NEW_SUPPLIER_FLOW, replace with is_legacy_brief_response logic

we want to retire the use of flask feature flags and this is one of the last uses of them. previously we were comparing the publishedAt date with a feature flag's enabled_since date to determine whether it's a "legacy brief" or not. instead use the logic of is_legacy_brief_response, adapted from brief-responses-frontend, to figure this out. this function works by inspecting a brief response, so doesn't work in cases where there are no responses. fortunately the only bit of template that want to know its legacy status make no sense if there aren't any brief responses, so that largely solves that. update tests to provide information needed by is_legacy_brief_response in mocks.

In a better world someone would have stored the information as to whether a brief belonged to the old flow or new flow on the actual model in the first place, but it's too late for that.

…_brief_response logic

we want to retire the use of flask feature flags and this is one of the last
uses of them. previously we were comparing the `publishedAt` date with
a feature flag's `enabled_since` date to determine whether it's a "legacy brief"
or not. instead use the logic of `is_legacy_brief_response`, adapted from
brief-responses-frontend, to figure this out. this function works by
inspecting a brief *response*, so doesn't work in cases where there are no
responses. fortunately the only bit of template that want to know its
legacy status make no sense if there aren't any brief responses, so that
largely solves that. update tests to provide information needed by
`is_legacy_brief_response` in mocks.
@risicle risicle requested review from katstevens and Wynndow August 24, 2018 11:52
brief_responses_required_evidence = (
None
if not brief_responses else
not is_legacy_brief_response(brief_responses[0], brief=brief)
Copy link
Contributor

Choose a reason for hiding this comment

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

Cool 👍I think it's a better explanation of what's happening than the alternative I just thought of...

... hardcoding a check for brief['publishedAt'][0:10] > '2017-02-08' - I mean, we wouldn't need separate checks for preview/staging as that data has long been wiped, but it feels a bit icky.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, mine still isn't an ideal way, in that None is falsey, but meh...

@risicle
Copy link
Contributor Author

risicle commented Aug 24, 2018

Just waiting for the OK on this one now...

Copy link
Contributor

@katstevens katstevens left a comment

Choose a reason for hiding this comment

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

Cool - was just checking about the response_counts stuff. I wonder if we should leave a comment say that's for the legacy flow as well.

@risicle risicle merged commit ec6d169 into master Aug 24, 2018
@risicle risicle deleted the ris-flask-0-12 branch August 24, 2018 14:23
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.

2 participants