[UI] Auto refresh/redirect experiment list/detail page when user selects a different namespace

[Bobgy]

Fixes #3296
/area frontend
/cc @chensun

Built image for preview:

gcr.io/gongyuan-pipeline-test/dev/frontend@sha256:60e2ee94c910f04a86e499a08ef73a9fc3dcbdba8160f5b03f8e2db1eed7a8f9

---

This change is 

[Bobgy]

/retest

[chensun]

Tested in my cluster, worked like a charm!
/lgtm

[chensun]

nit: not sure if this is just a common UI practice, feels a bit unreadable to me. Does it make sense to have a wrapper function with a more meaningful name?

[Bobgy]

I'm not sure what confused you. Do you mean this expect(xxxSpy).toHaveBeenLastCalledWith(...some arguments)?

This is fairly common, but previous implementation has been listing arguments (even though they are just default values).
I think if we change signature of listExperiment api method to let it accept an object of arguments, then the verifications here will include argument names, it will be a lot more readable.
e.g. proposed change will look like

expect(listExperimentsSpy).toHaveBeenLastCalledWith({
  page_token: xxx,
  page_size: xxx,
  sort_by: xxx,
  filter: xxx,
  resource_reference_key_type: xxx,
  resource_reference_key_id: xxx,
})

Above is a little more than what we need to do in this PR, so I will leave that to future. What do you think?

[chensun]

Sorry, somehow my comment was placed right above the line that I wanted to comment on.
I meant ...LIST_EXPERIMENT_DEFAULTS.slice(0, 4) wasn't very clear to me what it's trying to get with the slice part. I was thinking maybe have a helper function to hide it away, something like:

function getTestExperimentWithoutNamespace() {
  return [...LIST_EXPERIMENT_DEFAULTS.slice(0, 4)]
}

or maybe even better as:

function getTestExperiment(namespace='test-ns') {
  return [...LIST_EXPERIMENT_DEFAULTS.slice(0, 4), 'NAMESPACE', namespace]
}

Does it make sense? Just a nitpick. :)

[Bobgy]

Thanks for the explanation!
I don't quite like introducing another function, to keep it simple, I will just add another constant. please take a look what I changed

[chensun]

Yes, this looks good :)

[Bobgy]

@chensun Thanks for the review!
I've updated accordingly.

PTAL

[Bobgy]

I'm not sure what confused you. Do you mean this expect(xxxSpy).toHaveBeenLastCalledWith(...some arguments)?

This is fairly common, but previous implementation has been listing arguments (even though they are just default values).
I think if we change signature of listExperiment api method to let it accept an object of arguments, then the verifications here will include argument names, it will be a lot more readable.
e.g. proposed change will look like

expect(listExperimentsSpy).toHaveBeenLastCalledWith({
  page_token: xxx,
  page_size: xxx,
  sort_by: xxx,
  filter: xxx,
  resource_reference_key_type: xxx,
  resource_reference_key_id: xxx,
})

Above is a little more than what we need to do in this PR, so I will leave that to future. What do you think?

[chensun]

/lgtm

[chensun]

Sorry, somehow my comment was placed right above the line that I wanted to comment on.
I meant ...LIST_EXPERIMENT_DEFAULTS.slice(0, 4) wasn't very clear to me what it's trying to get with the slice part. I was thinking maybe have a helper function to hide it away, something like:

function getTestExperimentWithoutNamespace() {
  return [...LIST_EXPERIMENT_DEFAULTS.slice(0, 4)]
}

or maybe even better as:

function getTestExperiment(namespace='test-ns') {
  return [...LIST_EXPERIMENT_DEFAULTS.slice(0, 4), 'NAMESPACE', namespace]
}

Does it make sense? Just a nitpick. :)

[chensun]

/lgtm

[chensun]

Yes, this looks good :)

[Bobgy]

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Bobgy

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• frontend/OWNERS [Bobgy]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[Bobgy]

/retest