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

If you delete a Pipeline, the run reference leads to a meaningful message #1497

Merged
merged 1 commit into from
Jul 14, 2023

Conversation

pnaik1
Copy link
Contributor

@pnaik1 pnaik1 commented Jul 11, 2023

Closes: #1324

Description

#1324
The page displays the correct error message and a link to navigate to pipeline page
newPipeline

How Has This Been Tested?

  1. Create a Pipeline
  2. Create a Run for that Pipeline
  3. Delete the Pipeline
  4. View the Run on the Global Runs list page
  5. Click on the Pipeline link

Test Impact

no test coverage

Request review criteria:

Self checklist (all need to be checked):

  • The developer has manually tested the changes and verified that the changes work
  • The commits have meaningful messages (squashes happen on merge by the bot).
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has added tests or explained why testing cannot be added (unit tests & storybook for related changes)

If you have UI changes:

  • Included any necessary screenshots or gifs if it was a UI change.
  • Included tags to the UX team if it was a UI/UX change.

After the PR is posted & before it merges:

  • The developer has tested their solution on a cluster by using the image produced by the PR to main

@pnaik1 pnaik1 self-assigned this Jul 11, 2023
@pnaik1
Copy link
Contributor Author

pnaik1 commented Jul 11, 2023

cc @kywalker-rh can you take a look?

@andrewballantyne
Copy link
Member

@pnaik1 can we not avoid the header "Loading..." & the action items in the top right? ... perhaps we can replace the title with something else? I wonder if the breadcrumbs have value to take you back... maybe we can look into using React Router to send us back to where we came from?

This is a reasonable stop-gap though for showing an error screen like we did before.

I suspect Kyle will know more of what we can do.

Copy link
Contributor

@uidoyen uidoyen left a comment

Choose a reason for hiding this comment

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

/lgtm

@kywalker-rh
Copy link

kywalker-rh commented Jul 13, 2023

It seems weird to have the page still say "Loading...", my assumption from seeing this page is we are no longer loading, we've discovered it doesn't exist.

So in that case we should simply call the page "Pipeline not found". If we can remember the pipeline name from the link we should make it "Pipeline 'pipeline name' not found", and if we can't actually find the name we should remove the 'pipeline name' part (so in that case it would just be the existing text).

Hopefully that is clear.

Also, for the Actions button in the top right, we should simply disable it.

Copy link
Member

@andrewballantyne andrewballantyne left a comment

Choose a reason for hiding this comment

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

Perhaps we just simplify this and just show nothing but the 404?

Maybe we could mount the ApplicationPage, but make the body the 404 component you made. It would allow us to have some structure. Let me know if you want to talk about this.

I suggested two solutions as it really depends on how each looks. Maybe take screenshots of both or something.

@pnaik1
Copy link
Contributor Author

pnaik1 commented Jul 13, 2023

@andrewballantyne we can have a look

@pnaik1 pnaik1 force-pushed the issue-1324 branch 2 times, most recently from 3a41056 to 150e6a7 Compare July 14, 2023 07:53
@pnaik1
Copy link
Contributor Author

pnaik1 commented Jul 14, 2023

@andrewballantyne So I made the changes as per your request
withActionButton.webm
so you can observe that while loading, we have action button , So I think its not right to have this button , I have added additional condition in https://github.com/opendatahub-io/odh-dashboard/pull/1497/files#diff-153248b4472ef25aa4c01cf4f312bdddef50f7fe6052090df18ddd810437af90R87-R88
this is the final one Renewedpipeline.webm

this will also solve when we use the wrong template , even here we dont want the action button to be displayed
withActionButton

@kywalker-rh
Copy link

I'm pretty sure "Wrong pipeline" is your name of a test pipeline, but just want to double check we're not telling a user they have a wrong pipeline.

Also, I'm not totally following your thread, but if its significantly easier to simply not show the Actions button, instead of disabling it, I'm fine with that too.

@pnaik1
Copy link
Contributor Author

pnaik1 commented Jul 14, 2023

@kywalker-rh yeah the wrong pipeline is the pipeline name
So lets make it little simplified
I have made the changes and the changes look like this
newPipeline
you have suggested this one to remove the action button and the name change part

@kywalker-rh
Copy link

Looks good! Thanks for confirming the name. :)

@openshift-ci openshift-ci bot added the lgtm label Jul 14, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 14, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewballantyne, uidoyen

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:

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

@pnaik1
Copy link
Contributor Author

pnaik1 commented Jul 14, 2023

I have rebased the branch

@andrewballantyne
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Jul 14, 2023
@openshift-merge-robot openshift-merge-robot merged commit 4af4471 into opendatahub-io:main Jul 14, 2023
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: If you delete a Pipeline, the run reference leads to a weird error page
6 participants