Skip to content

upcoming: [M3-8025] - Placement Groups: Allow null for maximum_pgs_per_customer#10433

Merged
abailly-akamai merged 5 commits intolinode:developfrom
abailly-akamai:M3-8025
May 3, 2024
Merged

upcoming: [M3-8025] - Placement Groups: Allow null for maximum_pgs_per_customer#10433
abailly-akamai merged 5 commits intolinode:developfrom
abailly-akamai:M3-8025

Conversation

@abailly-akamai
Copy link
Contributor

@abailly-akamai abailly-akamai commented May 2, 2024

Description 📝

maximum_pgs_per_customer: This value can be unlimited for some customers in some regions, for which the API returns the null value. The UI representation of this null value is the "unlimited" string

Changes 🔄

  • Allow null for maximum_pgs_per_customer in APIv4
  • Add new util to return the proper limit in the UI

Target release date 🗓️

5/13/2024

Preview 📷

Screen Shot 2024-05-03 at 10 14 14

How to test 🧪

Verification steps

ℹ️ have both the "Placement Group" feature flag and MSW on

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

@abailly-akamai abailly-akamai self-assigned this May 2, 2024
@abailly-akamai abailly-akamai marked this pull request as ready for review May 2, 2024 15:13
@abailly-akamai abailly-akamai requested a review from a team as a code owner May 2, 2024 15:13
@abailly-akamai abailly-akamai requested review from carrillo-erik and dwiley-akamai and removed request for a team May 2, 2024 15:13
@github-actions
Copy link

github-actions bot commented May 2, 2024

Coverage Report:
Base Coverage: 81.82%
Current Coverage: 81.82%

Copy link
Contributor

@mjac0bs mjac0bs left a comment

Choose a reason for hiding this comment

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

The code and test coverage makes sense, so approving so as to make sure it gets in the upcoming release. I'm not a big fan of the text wrapping and way that unlimited is isolated on a line by itself, which makes it not very scannable and seem out of place.

I don't know that breaking the helper text earlier on that line would improve things, though, so maybe the lonely 'unlimited' is the best we can do, short of rendering a different sentence for the unlimited case, and my guess is that UX considered that and opted not to.

@abailly-akamai
Copy link
Contributor Author

@mjac0bs i completely agree - i made an executive decision to shorten that sentence which was too verbose anyway. (screenshot updated)

@carrillo-erik
Copy link
Contributor

Looks good to me. I was able to verify that the Toronto region will display unlimited and compared to New Jersey which has a limit of 5 Placement Groups.

Screenshot 2024-05-03 at 8 49 20 AM

Screenshot 2024-05-03 at 8 49 12 AM

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.

3 participants