feat(environments): Implement environment filter in stream view #6953
Conversation
| let access = new Set(org.access); | ||
| let allEnvironmentsLabel = t('All environments'); | ||
|
|
||
| // TODO: remove when feature is released |
There was a problem hiding this comment.
add your name to the TODO?
lynnagara
force-pushed
the
env-stream-view
branch
2 times, most recently
from
5a3ac13 to
ea18e50
Compare
Generated by 🚫 danger |
|
is this rdy to merge? |
|
@MaxBittker No, I'm gonna come back to this when we decide what to do with saved searches and environments. Labelled as WIP |
lynnagara
force-pushed
the
env-stream-view
branch
from
ea18e50 to
86511d9
Compare
lynnagara
force-pushed
the
env-stream-view
branch
2 times, most recently
from
2906ef9 to
439bcc8
Compare
There was a problem hiding this comment.
good to tag these with your name so you can find them more easily
lynnagara
force-pushed
the
env-stream-view
branch
2 times, most recently
from
425a16d to
6df0954
Compare
There was a problem hiding this comment.
the code itself looks good, and it's really important for dense, important logic like this to be rigorously tested. that being said, i know we had a lot of problems with the tests here I think it's pragmatic to safely merge this behind a feature flag, leave a commit message explaining the change in behavior, do manual testing, and follow up with fixed tests
|
|
||
| // you cannot apply both a query and a saved search (our routes do not | ||
| // support it), so the searchId takes priority | ||
| let nextSearchId = nextProps.params.searchId || null; |
There was a problem hiding this comment.
would you make this comment a tiny bit more verbose as well? this logic is complicated and i think we'll be glad in the future :o
|
|
||
| let url = this.getGroupListEndpoint(); | ||
|
|
||
| let query = this.state.query.replace(/^\s+|\s+$/g, ''); |
There was a problem hiding this comment.
example transform in a comment to explain the regex for people skimming?
| let query = this.state.query.replace(/^\s+|\s+$/g, ''); | ||
|
|
||
| let activeEnvironment = this.state.activeEnvironment; | ||
| let activeEnvName = activeEnvironment ? activeEnvironment.name : null; |
There was a problem hiding this comment.
i'd prefer this to be activeEnvironment && activeEnviroment.name unless the null is needed to make it work?
There was a problem hiding this comment.
It doesn't make a difference but I thought null is a more correct value than false
| }; | ||
|
|
||
| // Always keep the global active environment in sync with the queried environment | ||
| // The global environment wins unless there one is specified by the saved search |
There was a problem hiding this comment.
helpful comment, thank you 🗣
There was a problem hiding this comment.
although you you mind labeling the branches of the if? like moving your comment
The global environment wins unless there one is specified by the saved search
inside one of them to show explicity which branch it is. (nit )
| ...queryToObj(this.state.query), | ||
| environment: environment.name, | ||
| }); | ||
| } else { |
There was a problem hiding this comment.
again, maybe label these branches explicitly? i know the comment above is doing it, but labeled if branches help a lot when reading dense logic like this IMO
lynnagara
force-pushed
the
env-stream-view
branch
from
b25e8ba to
a8784dd
Compare
Also makes "All environments" the default if one is not set, and none of the names matches the list of likely production names