-
Notifications
You must be signed in to change notification settings - Fork 3
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
Artefact filters in url query parameters #139
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks good to me overall and I have no comments regarding it. One small edge case I noticed is how it works when you try to access a filter from the URL that is not in the available filters. My first intuition would be to ignore those, but the current way of handling treats them as a separate filter, so for example if I try to open the URL: http://localhost:46141/#/snaps?Status=some-random-status
it shows "some-random-status" as a status filter.
@andrejvelichkovski that comment makes it clear to me that you've really dug deep into this PR. Thanks, I really appreciate that! I will try to explain it thoroughly as this behavior isn't accidental. As you mentioned the edge case happens when the url has an option that's not present in any of the artefacts we've fetched from the backend. It's important to note that, this option could be valid, but it just so happens that none of the artefacts we got from the backend had it. So the question is what should we do in that case.
All of the above have issues. But the question goes down to either 1 or 3. It is also possible to add a request to backend to ask what are all the valid options. It would solve these issues as then we can only show this filter if it was actually valid. But I feel that it would add unnecessary complexity. I personally am a bit more inclined towards 3 than 1 because a user will likely copy a url instead of construct it themselves, and in that case the option shouldn't be misspelled or incorrect. But I do see the problem with it. wdyt? |
Oh, I now understand the full scope of the issue. I guess we have a set of "Available Filters", for example all users that can get assigned to an artefact, these are defined in our database, then we have a set of "Detected filters" which in the context of users are all users that are detected to be assigned to at least on one artefact, this is detected on the frontend. I didn't think of the fact that in the URL a user might specify for a filter that is "Available" but not "Detected". I'm also more inclined towards approach 3 you described. Most likely the users will not specify filters in URLs themselves, and even if they do it, when they see their filter appear, they will be "expecting" to see it. |
Ah I like the wording "Detected Filters", I'll update the PR to use it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! Let's land the change and announce it to the team, I'm sure people will find it useful to bookmark the URL to filter assigned artefacts to them :D
Resolves RTW-243.
Changes:
Video:
Screencast from 2024-03-15 16-20-54.webm