Feature flag to make inventory reports sysadmin only. (PP-1329)

[tdilauro]

Description

• Adds an Admin UI feature flag to restrict inventory reports to sysadmins (default: true = restrict)
• Adds a settings mechanism that can selectively restrict override from the environment.

Motivation and Context

Need ability to restrict inventory report requests from non-sysadmins until we've been able to test that functionality.

There were two things I was trying to accomplish:

• Have one home for configuration of feature flags; and
• Avoid accidentally overriding configuration that we have decided is fixed (see the hard-coded values I pulled back into the feature flags object).

Being able to prevent override by the environment variables preserves the existing behavior of those pre-existing feature flags (i.e., they could not be overridden by the environment -- they were fixed).

I believe the main use case for this is to avoid accidentally reconfiguring some functionality that we have decided is now 'fixed' into the system. I think should mostly apply only to functionality that is in transition. That said, we haven't gone back to the admin UI and removed these fixed feature flags. I'm not sure if that's because we want them there or because we just haven't done anything about them.

I made a separate class for this primarily to make updates to the feature flags a little clearer and less cluttered.

[Jira PP-1329]

How Has This Been Tested?

• Manually tested locally via API requests.
• All tests pass locally.
• CI tests pass.

Checklist

• N/A - I have updated the documentation accordingly.
• All new and existing tests passed.

[tdilauro]

Not sure that this is the best place for this, but it cannot go in the same file with ServiceConfiguration because of an import loop due to LoggerMixin.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.15%. Comparing base (3b27a67) to head (6626f10).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1898      +/-   ##
==========================================
+ Coverage   90.13%   90.15%   +0.01%     
==========================================
  Files         323      325       +2     
  Lines       39629    39692      +63     
  Branches     8630     8644      +14     
==========================================
+ Hits        35720    35784      +64     
  Misses       2602     2602              
+ Partials     1307     1306       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[jonathangreen]

Added a couple minor comments, but the code here looks good to me.

One question I have though, reading though this is what is the use case for flags that we can't override with environment variables? Why not just use ServiceConfiguration and allow enable_auto_list and show_circ_events_download to be overridden if desired?

[jonathangreen]

Minor: Should be able to remove this comment.

[tdilauro]

There were two things I was trying to accomplish:

• Have one home for configuration of feature flags; and
• Avoid accidentally overriding configuration that we have decided is fixed (see the hard-coded values I pulled back into the feature flags object).

Being able to prevent override by the environment variables preserves the existing behavior of those pre-existing feature flags (i.e., they could not be overridden by the environment -- they were fixed).

I believe the main use case for this is to avoid accidentally reconfiguring some functionality that we have decided is now 'fixed' into the system. I think should mostly apply only to functionality that is in transition. That said, we haven't gone back to the admin UI and removed these fixed feature flags. I'm not sure if that's because we want them there or because we just haven't done anything about them.

I made a separate class for this primarily to make updates to the feature flags a little clearer and less cluttered.

[jonathangreen]

I'm fine with this going in as is, so feel free either way.

My opinion on this though is that we should allow the feature flags to be overridden by env vars and any of the flags we want to be set as fixed, we can put in tickets to remove the flag and clean up the config in the admin ui and CM.

[jonathangreen]

Would it make sense for this to live in palace.manager.service.configuration with the parent ServiceConfiguration class. Rather then in palace.manager.util.settings?

[tdilauro]

Yeah. Probably. I wasn't entirely sure where to put it, but that makes sense.

[tdilauro]

Though, as I noted above, it cannot live in the same file:

Not sure that this is the best place for this, but it cannot go in the same file with ServiceConfiguration because of an import loop due to LoggerMixin.

[jonathangreen]

Maybe we could move palace.manager.service.configuration.ServiceConfiguration to palace.manager.service.configuration.base.ServiceConfiguration and put this in the palace.manager.service.configuration package. We can just leave it where it is for now as well.

[tdilauro]

@jonathangreen I didn't see this before I moved them. I put both of them in palace.manager.service.configuration package directory. I can move them, though, if you'd prefer.

[jonathangreen]

No whats here looks good to me.