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.

Sign up for GitHub

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

Jump to bottom

upcoming: [M3-7696] - Edit Access key Drawer - Fix Save button is disabled #10118

Merged
merged 15 commits into from
Feb 7, 2024
Merged

upcoming: [M3-7696] - Edit Access key Drawer - Fix Save button is disabled #10118

merged 15 commits into from
Feb 7, 2024

Conversation

cpathipa
Copy link
Contributor

@cpathipa cpathipa commented Jan 29, 2024 •
edited
Loading

Description 📝

  • Fix Save button is disabled in Edit Access Key Drawer
  • Copy update - "Edit Access Key" .
  • Copy update - Action menu - "Edit" instead of "Edit Label".

Preview 📷

Before After
image image

How to test 🧪

Prerequisites

(How to setup test environment)

  • Turn on MSW
  • Turn on OBJ Multi-Cluster flag.

Verification steps

(Turn on feature flag)

  • Verify the functionality of disable button, when no changes in Label and region
  • Verify the functionality of disable button, when changes in Label / region / both. Also, verify payload w.r.t changes and API spec.
  • Verify copy updates in Edit Access key drawer title and action menu.

(Turn off feature flag)

  • Verify the functionality of disable button, when no changes in Label.
  • Verify there is no regression in Edit / Create Access key flow.

As an Author I have considered 🤔

Check all that apply

  • 👀 Doing a self review
  • ❔ Our contribution guidelines
  • 🤏 Splitting feature into small PRs
  • ➕ Adding a changeset
  • 🧪 Providing/Improving test coverage
  • 🔐 Removing all sensitive information from the code and PR description
  • 🚩 Using a feature flag to protect the release
  • 👣 Providing comprehensive reproduction steps
  • 📑 Providing or updating our documentation
  • 🕛 Scheduling a pair reviewing session
  • 📱 Providing mobile support
  • ♿ Providing accessibility support

@cpathipa cpathipa requested a review from a team as a code owner January 29, 2024 16:07
@cpathipa cpathipa requested review from hana-akamai and abailly-akamai and removed request for a team January 29, 2024 16:07
@cpathipa cpathipa marked this pull request as draft January 29, 2024 16:08
@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 29, 2024 •
edited
Loading

Coverage Report: ✅
Base Coverage: 81.18%
Current Coverage: 81.18%

@cpathipa cpathipa self-assigned this Jan 29, 2024
@cpathipa cpathipa marked this pull request as ready for review January 29, 2024 18:30
@jdamore-linode jdamore-linode mentioned this pull request Feb 5, 2024
Merged
12 tasks
jdamore-linode
jdamore-linode approved these changes Feb 5, 2024
View reviewed changes
Copy link
Contributor

@jdamore-linode jdamore-linode left a comment

Choose a reason for hiding this comment

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

Agreed with Alban's suggestion about the Formik types, but otherwise this looks good. Thanks @cpathipa!

packages/manager/src/features/ObjectStorage/AccessKeyLanding/utils.ts Show resolved Hide resolved
dwiley-akamai
dwiley-akamai reviewed Feb 6, 2024
View reviewed changes
Copy link
Contributor

@dwiley-akamai dwiley-akamai left a comment

Choose a reason for hiding this comment

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

UI and functionality with feature flag off ✅
Modified UI and functionality (incl. expected payloads upon save) with feature flag on ✅

packages/manager/.changeset/pr-10118-upcoming-features-1706544516803.md Outdated Show resolved Hide resolved
packages/validation/src/objectStorageKeys.schema.ts Outdated Show resolved Hide resolved
packages/manager/src/features/ObjectStorage/AccessKeyLanding/OMC_AccessKeyDrawer.tsx Outdated Show resolved Hide resolved
packages/manager/src/features/ObjectStorage/AccessKeyLanding/utils.test.ts Outdated Show resolved Hide resolved
hana-akamai
hana-akamai reviewed Feb 6, 2024
View reviewed changes
Copy link
Contributor

@hana-akamai hana-akamai left a comment

Choose a reason for hiding this comment

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

If I turn off the MSW with the OBJ Multi-Cluster flag still on, the page crashes. Can we handle this error more gracefully?

Screen.Recording.2024-02-06.at.4.07.46.PM.mov

packages/manager/src/features/ObjectStorage/AccessKeyLanding/OMC_AccessKeyDrawer.tsx
isRestrictedUser ||
(mode !== 'creating' &&
objectStorageKey &&
!hasLabelOrRegionsChanged(formik.values, objectStorageKey));
Copy link
Contributor

Choose a reason for hiding this comment

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

Wondering if we can pass in initialValues here instead of objectStorageKey?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree it would be a nice improvement - it's making reviewers think something is quite broken

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think we are updating the initialValues with objectStorageKey in edit mode.

abailly-akamai
abailly-akamai approved these changes Feb 7, 2024
View reviewed changes
Copy link
Contributor

@abailly-akamai abailly-akamai left a comment

Choose a reason for hiding this comment

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

Thanks for the type improvements and the improved coverage!

All good to go, confirmed changes in the UI as well ✅

packages/manager/src/features/ObjectStorage/AccessKeyLanding/OMC_AccessKeyDrawer.tsx Show resolved Hide resolved
@cpathipa
Copy link
Contributor Author

cpathipa commented Feb 7, 2024

If I turn off the MSW with the OBJ Multi-Cluster flag still on, the page crashes. Can we handle this error more gracefully?

Screen.Recording.2024-02-06.at.4.07.46.PM.mov

@HanaXu Yes, this issue is on my radar and will be addressed with M3-7743. Until then, we should enable the MSW first, then the OBJ feature flag.

@hana-akamai hana-akamai added the Approved Multiple approvals and ready to merge! label Feb 7, 2024
@cpathipa cpathipa merged commit 165b5bf into linode:develop Feb 7, 2024
18 checks passed
@cpathipa cpathipa deleted the M3-7696 branch February 7, 2024 15:42
@cpathipa cpathipa mentioned this pull request Feb 12, 2024
Closed
This was referenced Feb 20, 2024
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abailly-akamai abailly-akamai abailly-akamai approved these changes

@hana-akamai hana-akamai hana-akamai approved these changes

@jdamore-linode jdamore-linode jdamore-linode approved these changes

@dwiley-akamai dwiley-akamai Awaiting requested review from dwiley-akamai

Assignees

@cpathipa cpathipa

Labels
Approved Multiple approvals and ready to merge! OBJ Multi-Cluster
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@cpathipa @jdamore-linode @dwiley-akamai @hana-akamai @abailly-akamai