[RW-13480][risk=no] Workspace Banners for Initial Credits #8951

evrii · 2024-11-18T20:06:47Z

There are several different banner variants here. I tried to err on the side of readability even if that meant that the code was a little longer. With that said, I am certainly open to feedback if you have thoughts on how the code can be better organized.

In order to show each of these banners, you will need to manipulate five different states:

isCreator - whether or not you created the workspace that you are viewing
isExpired - whether the user's initial credit expiration date is in the past
isExpiringSoon - whether the user's initial credit expiration date is within X(initialCreditsExpirationWarningDays in config) days of expiring. This state is always checked after isExpired so it is checking: now>= expirationDate - initialCreditsExpirationWarningDays && now<expirationDate
isExhausted - whether or not a user has used all their credits
isEligible - whether or not a user is eligible for an extension of their initial credit expiration date: now >= expirationDate - initialCreditsExpirationWarningDays

Creator Expiring Soon Eligible for Extension
!isExpired && isExpiringSoon && !isExhausted && isEligible && isCreator

Non-creator Expiring Soon Eligible for Extension
!isExpired && isExpiringSoon && !isExhausted && isEligible && !isCreator

Creator Expired Eligible for Extension
isExpired && !isExhausted && isEligible && isCreator

[Non-creator Expired Eligible for Extension]
isExpired && !isExhausted && isEligible && !isCreator

Creator Expired Not Eligible for Extension
(isExpired || isExhausted) && !isEligible && isCreator

Non-creator Expired Not Eligible for Extension
(isExpired || isExhausted) && !isEligible && !isCreator

PR checklist

evrii · 2024-11-18T21:44:00Z

ui/src/app/pages/workspace/invalid-billing-banner.tsx

+    plusDays(workspace.initialCredits.expirationEpochMillis, 5) < Date.now();
+  let message;
+  let title;
+  if (isExpiringSoon && isEligibleForExtension) {


This feels complicated, but the only way that I can think of simplifying it is to break up individual phrases into variables and then have several ternary statements. I feel like that might be more efficient, but I'm not sure it would be more readable.

I would love thoughts here.

your comment refers to an outdated version. is there a similar comment for the updated version?

…8941)

….6 in /ui (#8953) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

evrii · 2024-11-20T21:45:45Z

ui/src/app/pages/workspace/invalid-billing-banner.tsx

-      zIndex={500}
-    />
+    <>
+      {((isExpiringSoon && isEligibleForExtension) || isExpired) && (


There is no reason to show this banner if one of these conditions is not true.

This component is only used by the breadcrumbs component. So this condition could be shifted up, but these conditions would need to be recalculated. In that case, if appropriate, breadcrumbs could calculate these values and then pass them down.

The only other usage of workspace in this component is the construction of the link associated with the "Edit Workspace" button.

In that case, why is isExhausted a parameter to the text functions used to construct message? If only isExhausted is true, then we don't need message.

jmthibault79 · 2024-11-25T21:18:53Z

api/src/main/java/org/pmiops/workbench/utils/mappers/WorkspaceMapper.java

+  @Mapping(
+      target = "initialCredits.extensionEpochMillis",
+      source = "dbWorkspace.creator",
+      qualifiedByName = "getInitialCreditsExpiration")


this sets extension to the same value as expiration. Is this intentional?

jmthibault79 · 2024-11-25T21:20:21Z

ui/src/app/pages/workspace/invalid-billing-banner.tsx

+    plusDays(workspace.initialCredits.expirationEpochMillis, 5) < Date.now();
+  let message;
+  let title;
+  if (isExpiringSoon && isEligibleForExtension) {


your comment refers to an outdated version. is there a similar comment for the updated version?

jmthibault79 · 2024-11-25T21:32:10Z

ui/src/app/pages/workspace/invalid-billing-banner.tsx

@@ -19,23 +23,167 @@ interface Props extends NavigationProps {
  onClose: Function;
 }

+const InitialCreditsArticleLink = () => (


these 2 links have different text but the same URL. that does not seem right.

jmthibault79 · 2024-11-25T21:37:11Z

ui/src/app/pages/workspace/invalid-billing-banner.tsx

+  const isExpiringSoon =
+    workspace &&
+    !isExpired &&
+    plusDays(


could use your new minusDays here

jmthibault79 · 2024-11-25T21:46:14Z

ui/src/app/pages/workspace/invalid-billing-banner.tsx

+) => {
+  const whose = whoseCredits(isCreator);
+  let whatIsHappening: string;
+  if (isExhausted || (isExpired && !isEligibleForExtension)) {


I think you were looking for a way to simplify the logic? I like these function names and I think the outputs make sense, so I'd keep those.

Maybe one way to simplify the logic would involve an enum of possible states, named something like ExtensionCondition. Before calling these text functions, we would call another function determineCondition() to generate the enum. then these text functions become switch or cond statements, something like:

case EXHAUSTED | EXPIRED_CANNOT_EXTEND: ... case EXPIRED_CAN_EXTEND: ... case EXPIRING_SOON_OWNER_CAN_EXTEND | EXPIRING_SOON_NONOWNER_CAN_EXTEND: ...

I don't know that this would actually be better.

jmthibault79 · 2024-11-25T21:50:07Z

ui/src/app/pages/workspace/invalid-billing-banner.tsx

+    whatIsHappening = 'have run out.';
+  } else if (isExpired && isEligibleForExtension) {
+    whatIsHappening = 'have expired.';
+  } else if (isExpiringSoon && isEligibleForExtension) {


what happens in the case of isExpiringSoon && !isEligibleForExtension? If that never happens here, do you mind adding a comment?

jmthibault79 · 2024-11-25T21:54:51Z

ui/src/app/pages/workspace/invalid-billing-banner.tsx

+  } else {
+    return (
+      <>
+        {isEligibleForExtension && (isExpired || isExpiringSoon) && (


because this section does not mention adding a billing account, the Edit Workspace button lacks the context that explains why it's there. From a program perspective, adding a billing account is the preferred outcome, so I think we should suggest that whenever possible.

jmthibault79 · 2024-11-25T22:02:27Z

ui/src/app/components/invalid-billing-banner.spec.tsx

+    render(
+      <MemoryRouter>
+        <InvalidBillingBanner
+          {...{ signatureState }}


this doesn't look right. None of these props are used by InvalidBillingBanner.

jmthibault79 · 2024-11-25T22:03:52Z

ui/src/app/components/invalid-billing-banner.spec.tsx

+  beforeEach(() => {
+    registerApiClient(ProfileApi, new ProfileApiStub());
+
+    // Do I need this?


I don't think so

jmthibault79 · 2024-11-25T22:04:55Z

ui/src/app/components/invalid-billing-banner.spec.tsx

+    // Set expiration date to be in the past if expired, in the future if not.
+    // If expiring soon, set it to be within the warning threshold, and just outside otherwise.
+    // Expired and expiringSoon are mutually exclusive.
+    const daysUntilExpiration = expired


I like this

jmthibault79 · 2024-11-25T22:07:20Z

ui/src/app/components/invalid-billing-banner.spec.tsx

+  };
+
+  it('should show expiring soon banner to user who created the workspace', async () => {
+    setupWorkspace(false, false, true, true);


I'd prefer to see these made explicit, like

const exhausted = false; const expired = false; const expiringSoon = true; const ownedByMe = true; setupWorkspace(exhausted, expired, expiringSoon, ownedByMe);

jmthibault79 · 2024-11-25T22:11:21Z

ui/src/app/components/invalid-billing-banner.spec.tsx

+    );
+  });
+
+  it('should show expired banner to user who did not create the workspace and the owner is not eligible for extension', async () => {


I would like to see "exhausted" tests as well

evrii added 10 commits November 12, 2024 15:28

Added basic tests.

40bdb61

Add expiration date.

cc7918c

Update styling

dee730b

Add checks for extension button.

e76615e

Add extension and info buttons.

a5ff2f3

Merge remote-tracking branch 'origin/main' into eric/RW-13481

0adb7e4

Merge remote-tracking branch 'origin/main' into eric/RW-13481

ad412f5

Added extension field.

c214129

First pass at banners

cd7f7d3

Simplified logic?

bdd7f49

evrii commented Nov 18, 2024

View reviewed changes

evrii added 5 commits November 18, 2024 16:54

Added config values.

a2dbc9d

[RW-13735][risk=no] Added modal to trigger initial credit extension. (#…

42b159c

…8941)

Merge remote-tracking branch 'origin/eric/RW-13481' into eric/RW-13480

f50fb7b

Fix title and eligibility usage

bd12bb9

Merge remote-tracking branch 'origin/main' into eric/RW-13480

7b35bd6

evrii changed the base branch from main to nsaxena/FixPuppeterTest November 19, 2024 17:02

evrii changed the base branch from nsaxena/FixPuppeterTest to main November 19, 2024 17:03

evrii closed this Nov 19, 2024

evrii reopened this Nov 19, 2024

dependabot bot and others added 10 commits November 19, 2024 13:18

[risk=low][no ticket] [dependabot] Bump cross-spawn from 7.0.3 to 7.0…

6480a2f

….6 in /ui (#8953) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

laucn multi-zone (#8956)

7881475

remove getUploadResult in verifyAndLog (#8955)

92a2784

Merge remote-tracking branch 'origin/main' into eric/RW-13480

0abe313

Update banner

0f0326e

Cleaner banner?

8092dba

Merge remote-tracking branch 'origin/main' into eric/RW-13480

e507305

Cleanup

d7ec332

Add expiring soon banners.

34957ec

Add expired but eligible tests

d1b1f87

evrii added 2 commits November 20, 2024 16:30

Add expired and ineligible tests

f463eec

Hide banner if not appropriate

73378ea

evrii commented Nov 20, 2024

View reviewed changes

evrii added 5 commits November 22, 2024 09:18

Cleanup

3c286c7

Cleanup

cbca190

Updated language

cd304ee

Updated spacing

300240f

Updated test email.

3c6ec5e

evrii changed the title ~~Eric/rw 13480~~ [RW-13480][risk=no] Workspace Banners for Initial Credits Nov 22, 2024

evrii marked this pull request as ready for review November 22, 2024 21:29

jmthibault79 reviewed Nov 25, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RW-13480][risk=no] Workspace Banners for Initial Credits #8951

[RW-13480][risk=no] Workspace Banners for Initial Credits #8951

evrii commented Nov 18, 2024 •

edited

Loading

evrii Nov 18, 2024

jmthibault79 Nov 25, 2024 •

edited

Loading

evrii Nov 20, 2024

jmthibault79 Nov 25, 2024

jmthibault79 Nov 25, 2024

jmthibault79 Nov 25, 2024 •

edited

Loading

jmthibault79 Nov 25, 2024

jmthibault79 Nov 25, 2024

jmthibault79 Nov 25, 2024

jmthibault79 Nov 25, 2024

jmthibault79 Nov 25, 2024

jmthibault79 Nov 25, 2024

jmthibault79 Nov 25, 2024

jmthibault79 Nov 25, 2024

jmthibault79 Nov 25, 2024

jmthibault79 Nov 25, 2024

[RW-13480][risk=no] Workspace Banners for Initial Credits #8951

Are you sure you want to change the base?

[RW-13480][risk=no] Workspace Banners for Initial Credits #8951

Conversation

evrii commented Nov 18, 2024 • edited Loading

Choose a reason for hiding this comment

jmthibault79 Nov 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmthibault79 Nov 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evrii commented Nov 18, 2024 •

edited

Loading

jmthibault79 Nov 25, 2024 •

edited

Loading

jmthibault79 Nov 25, 2024 •

edited

Loading