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

fix: [M3-8164] - regions length check #10519

Merged
merged 3 commits into from
May 29, 2024
Merged

fix: [M3-8164] - regions length check #10519

merged 3 commits into from
May 29, 2024

Conversation

cpathipa
Copy link
Contributor

@cpathipa cpathipa commented May 28, 2024 •
edited
Loading

Description 📝

This PR checks the length of regions before reading its data while rending Table cell in Access Key table.

Target release date 🗓️

6/7

How to test 🧪

Prerequisites

(How to setup test environment)

  • Use dev env and dev account with appropriate account tags.(reach me if you need help on account tags).

Reproduction steps

(How to reproduce the issue, if applicable)

Verification steps

(How to verify changes)

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 May 28, 2024 17:42
@cpathipa cpathipa requested review from bnussman-akamai and jaalah-akamai and removed request for a team May 28, 2024 17:42
@cpathipa cpathipa self-assigned this May 28, 2024
@cpathipa cpathipa marked this pull request as draft May 28, 2024 17:42
@cpathipa cpathipa marked this pull request as ready for review May 28, 2024 17:51
@github-actions GitHub Actions
Copy link

github-actions bot commented May 28, 2024 •
edited
Loading

Coverage Report: ✅
Base Coverage: 82.29%
Current Coverage: 82.29%

dwiley-akamai
dwiley-akamai approved these changes May 28, 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.

Length check prevents the crash ✅

...ges/manager/src/features/ObjectStorage/AccessKeyLanding/AccessKeyTable/HostNameTableCell.tsx Outdated
@@ -28,7 +28,7 @@ export const HostNameTableCell = ({

const { regions } = storageKeyData;

if (!regionsLookup || !regionsData || !regions) {
if (!regionsLookup || !regionsData || !regions.length) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Our length checks are normally more explicit, i.e., regions.length === 0.

Did UX share any ideas other than an empty cell for these cases? Maybe "None" or "N/A"?

Copy link
Member

Choose a reason for hiding this comment

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

++ for the explicit length check (regions.length === 0)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call @dwiley-akamai None makes sense to me. Looks like we are using it in VPC as well. However, I will confirm with UX and get back on this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

UX is confirmed on showing 'None'.

@cpathipa cpathipa merged commit 6f013a5 into linode:develop May 29, 2024
18 checks passed
bnussman-akamai pushed a commit that referenced this pull request May 30, 2024
@cpathipa @bnussman
fix: [M3-8164] - regions length check (#10519)
1175c90 
* fix: [M3-8164] - regions length check

* Added changeset: fix regions length check in HostNameTableCell

* PR feedback: @bnussman and @DevDW
@bnussman-akamai bnussman-akamai mentioned this pull request May 30, 2024
Closed
@jaalah-akamai jaalah-akamai mentioned this pull request Jun 6, 2024
Merged
@cpathipa cpathipa mentioned this pull request Jun 10, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dwiley-akamai dwiley-akamai dwiley-akamai approved these changes

@bnussman-akamai bnussman-akamai bnussman-akamai approved these changes

@jaalah-akamai jaalah-akamai Awaiting requested review from jaalah-akamai jaalah-akamai is a code owner automatically assigned from linode/frontend

Assignees

@cpathipa cpathipa

Labels
OBJ Multi-Cluster
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@cpathipa @dwiley-akamai @bnussman-akamai