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

[DT-1001] select billing profile #1735

Merged
merged 6 commits into from
Jan 15, 2025

Conversation

s-rubenstein
Copy link
Contributor

Screen.Recording.2025-01-10.at.2.00.02.PM.mov

@s-rubenstein s-rubenstein requested a review from a team as a code owner January 10, 2025 19:01
@s-rubenstein s-rubenstein requested review from rushtong and rjohanek and removed request for a team January 10, 2025 19:01
@s-rubenstein s-rubenstein changed the title Sr/dt 1001 select billing profile [DT-1001] select billing profile Jan 10, 2025
Copy link

cypress bot commented Jan 10, 2025

jade-data-repo-ui    Run #3963

Run Properties:  status check passed Passed #3963  •  git commit 8546e462b2 ℹ️: Merge 8ebaa8a1d8ff424a10fcbe3888cb72a57d0b492b into 0c571d9e12a7c67226bd2cc85f2d...
Project jade-data-repo-ui
Branch Review sr/dt-1001-select-billing-profile
Run status status check passed Passed #3963
Run duration 02m 51s
Commit git commit 8546e462b2 ℹ️: Merge 8ebaa8a1d8ff424a10fcbe3888cb72a57d0b492b into 0c571d9e12a7c67226bd2cc85f2d...
Committer Sky Rubenstein
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 0
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 17
View all changes introduced in this branch ↗︎

Copy link
Contributor

@rjohanek rjohanek left a comment

Choose a reason for hiding this comment

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

this looks good, but looking at the ticket, I think we talked about adding the name and description of the snapshot to the modal as well. Did we still want to do that as part of this work?

@s-rubenstein
Copy link
Contributor Author

@rjohanek oh I see that now - we should work on updating ticket names when we make those sorts of changes as well.

Copy link
Contributor

@rushtong rushtong left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me 👍🏽

@s-rubenstein
Copy link
Contributor Author

Screen.Recording.2025-01-13.at.4.38.15.PM.mov

id="snapshot-name"
label="Snapshot Name"
value={snapshotName}
onChange={(e) => setSnapshotName(e.target.value)}
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to do input validation here for name and description because we are sending it to the backend?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The backend has validation - I checked other places and TDR UI doesn't currently validate inputs - it just pushes the problem to the API. We probably want clientside validation at some point, but that should be out of scope of this. (That's also when I would add more testing)

@lstrano-broad
Copy link

LGTM! Thx

Copy link

Copy link
Contributor

@rjohanek rjohanek left a comment

Choose a reason for hiding this comment

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

thanks!

@s-rubenstein s-rubenstein merged commit cc006d3 into develop Jan 15, 2025
9 checks passed
@s-rubenstein s-rubenstein deleted the sr/dt-1001-select-billing-profile branch January 15, 2025 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants