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

Expose status_info in the UI #332

Merged
merged 4 commits into from
Nov 16, 2023

Conversation

nkaretnikov
Copy link
Contributor

@nkaretnikov
Copy link
Contributor Author

This shouldn't be merged before the companion PR lands on the server side.

@nkaretnikov
Copy link
Contributor Author

nkaretnikov commented Nov 7, 2023

Looks like this (prints status_info in parens if available - "build_path too long" on the screenshot here):

Before:
Screen Shot 2023-11-07 at 21 38 55

After:
Screen Shot 2023-11-07 at 20 46 10

@nkaretnikov
Copy link
Contributor Author

This has many changes due to formatting. It helps to hide whitespace when reviewing:
Screen Shot 2023-11-07 at 21 03 27

@nkaretnikov
Copy link
Contributor Author

nkaretnikov commented Nov 7, 2023

@steff456 I still need to figure out why playwright fails, but is this cool overall? UPD: playwright might be just a flaky test, resolved here: #328.
@smeragoel Any comments on the design (see screenshot)?

@steff456
Copy link
Contributor

steff456 commented Nov 7, 2023

@nkaretnikov can you please add a screenshot before the change? In that way it is clearer what's the difference in the UI

@nkaretnikov
Copy link
Contributor Author

@steff456 #332 (comment) Added before/after.

@nkaretnikov nkaretnikov force-pushed the prefix-size-check-649 branch from cb66efa to 5408bba Compare November 8, 2023 10:52
Nikita Karetnikov added 2 commits November 8, 2023 19:43
This now shows an error message in the UI when the environment name is
too long for a DB field (longer than 255 characters).
setError({
message: data.message,
message: e?.data?.message ?? createLabel(undefined, "error"),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This used to fail because data was undefined.

A repro: try to create an environment with 256 a chars as the env name: 'a' * 256. This causes a SQL error on the server because the field is only 255 chars long. With this change, the UI now shows an error message.

I'm not adding a "before" screenshot because before it was just showing nothing - same page you have when you click Create. You could also see an uncaught exception in the dev browser console (that's usually opened by pressing f12).

Screen Shot 2023-11-08 at 21 06 21

Copy link
Contributor

Choose a reason for hiding this comment

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

What are the possible error messages that will be displayed here?

Copy link
Contributor Author

@nkaretnikov nkaretnikov Nov 12, 2023

Choose a reason for hiding this comment

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

e?.data?.message corresponds to what used to be here before. I think message is not actually used by the said route, but I would keep it just in case, in case it's changed later. The route on the server is:

api_post_specification
...
return {"status": "ok", "data": {"build_id": build_id}}

Unlike some other routes, it doesn't include the message field.

The new message is always a generic error message shown on the screenshot above. This is what we do elsewhere as well, so it's consistent.

@nkaretnikov nkaretnikov marked this pull request as ready for review November 8, 2023 20:13
@nkaretnikov
Copy link
Contributor Author

@steff456 PTAL. Tests now pass, not planning to make more changes.

Do not merge this yet. The companion server PR (conda-incubator/conda-store#653) is not reviewed yet. I'll merge this myself when that one is merged.

Copy link
Contributor

@steff456 steff456 left a comment

Choose a reason for hiding this comment

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

I just left a minor comment related to the code. I think this PR first needs approval from @smeragoel to check the design first. This is the first element that will show actual error messages in the UI, so it is important to have a design system for them and also check if we want to have the raw error message displayed.

setError({
message: data.message,
message: e?.data?.message ?? createLabel(undefined, "error"),
Copy link
Contributor

Choose a reason for hiding this comment

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

What are the possible error messages that will be displayed here?

@nkaretnikov
Copy link
Contributor Author

  • Replied to @steff456's comments. I don't think there's anything to be changed here.
  • Waiting for @smeragoel to comment on the design. Alternatively, we can just merge this and address design concerns later. It doesn't look like a huge change.
  • Before we merge, waiting until the companion PR on the server side is approved.

@nkaretnikov nkaretnikov requested a review from steff456 November 12, 2023 21:08
Copy link

@smeragoel smeragoel left a comment

Choose a reason for hiding this comment

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

No major design changes, approved from my side!

Copy link
Contributor

@steff456 steff456 left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @nkaretnikov

@nkaretnikov
Copy link
Contributor Author

Before we merge, waiting until the companion PR on the server side is approved.

@nkaretnikov nkaretnikov merged commit 5ef8cac into conda-incubator:main Nov 16, 2023
2 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.

4 participants