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 fully enabled feature flags that have been enabled for years #3375

Merged
merged 9 commits into from
Feb 21, 2024

Conversation

murny
Copy link
Contributor

@murny murny commented Feb 11, 2024

#3277

This PR removes all the fully enabled feature flags that have been enabled for years:

image

datacite_api flag was removed here: #3277
or_facts feature flag I think is a duplicate and probably never worked

Note: There may be good reason to keep these flags around, such as being able to quickly switch back to old behaviour for some reason? So if so, let me know and we can unremoved these.

After this is merged, we will have to remove these from Flipper UI in production as well.

@@ -3,24 +3,10 @@
class SolrBadRequestTest < ActionDispatch::IntegrationTest

test 'request in which solr gives 400 gives a proper response' do
get search_url, params: { facets: { all_subjects_sim: ['Antimicrobial', '"blown-pack"'] } }
Copy link
Contributor Author

@murny murny Feb 12, 2024

Choose a reason for hiding this comment

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

These are passing now, so might have to do with :or_facets feature flag being enabled now.

The only one failing now is the last one which had special characters ("\"\\'Bacteriocins").

So I assume it was failing before because we passing an array or a hash with multiple values and now with :or_facets feature flag being always enabled this is okay?

So just simplify the test and added a comment on what we are actually testing here.

@murny murny marked this pull request as ready for review February 12, 2024 17:58
Copy link
Contributor

@ConnorSheremeta ConnorSheremeta left a comment

Choose a reason for hiding this comment

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

LGTM. I wonder if keeping the batch_ingest flag would be worth it to restrict that feature to certain actors (a subset of admins)... however, I don't think that there's much benefit to that.

pgwillia
pgwillia previously approved these changes Feb 14, 2024
Copy link
Member

@pgwillia pgwillia left a comment

Choose a reason for hiding this comment

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

I spoke with @leahvanderjagt yesterday about this change. She agrees that these are all behaviours we want to keep long term. 👍

Really good work to keep our code clean so what we're maintaining is as simple as possible!

@murny
Copy link
Contributor Author

murny commented Feb 16, 2024

@pgwillia Awesome, thanks for confirming with Leah 🙏

@murny murny dismissed stale reviews from pgwillia and ConnorSheremeta via 337b84f February 16, 2024 18:32
Copy link
Contributor

@ConnorSheremeta ConnorSheremeta left a comment

Choose a reason for hiding this comment

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

👍 Just a small formatting error from rebasing, could be resolved in an upcoming PR

CHANGELOG.md Outdated
@@ -9,6 +9,9 @@ New entries in this file should aim to provide a meaningful amount of informatio

## [Unreleased]

### Removed
- Remove fully enabled feature flags that have been enabled for years [PR#3375](https://github.com/ualbertalib/jupiter/pull/3375)
-
Copy link
Contributor

Choose a reason for hiding this comment

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

Small formatting error from the rebase

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops good catch, will fix this

pgwillia
pgwillia previously approved these changes Feb 21, 2024
@murny murny dismissed stale reviews from pgwillia and ConnorSheremeta via 22b64cc February 21, 2024 17:14
Copy link
Contributor

@ConnorSheremeta ConnorSheremeta left a comment

Choose a reason for hiding this comment

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

👍

@murny murny merged commit 86c425e into master Feb 21, 2024
3 checks passed
@murny murny deleted the remove-fully-enabled-feature-flags branch February 21, 2024 17:50
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