feat: [M3-7158] - Update NodeJS naming to Node.js for Marketplace #11086

carrillo-erik · 2024-10-10T16:56:31Z

Description 📝

Updates any reference to NodeJS in Marketplace to Node.js.

Changes 🔄

List any change relevant to the reviewer.

Text copy in updated to include punctuation and lower case for js.

Preview 📷

Before	After

How to test 🧪

Verification steps

(How to verify changes)

Visit the Marketplace tab and inspect the text copy found by clicking the info icon (i) under the following Apps.
- NodeJS
- OpenLiteSpeed NodeJS
Verify that all text copy references of NodeJS have been replaced with Node.js within the AppDetailDrawer.

As an Author I have considered 🤔

Check all that apply

github-actions · 2024-10-10T17:24:11Z

Coverage Report: ✅
Base Coverage: 87.06%
Current Coverage: 87.06%

carrillo-erik · 2024-10-10T17:25:23Z

I believe the text for the headers found in the AppSelectionCard are configured via LaunchDarkly. If this is correct, than no further code changes will be required for this PR.

bnussman-akamai · 2024-10-11T14:08:43Z

The titles in the AppSelectionCard.tsxs actually come directly from the StackScripts endpoint. The AppCard's title is just stackscript.label with some transformation done by getMarketplaceAppLabel.

I'm thinking that, rather than updating the hardcoded name on our end, we might want update the AppDetailsDrawer to show the StackScript label rather than the hardcoded title so that the marketplace team can update what the API returns and fix title issues without any code changes.

Let me know if you have any questions on that ⬆️

The changes in the description and guides still have to be done hard-coded so those changes look good to me.

carrillo-erik · 2024-10-11T14:48:54Z

The titles in the AppSelectionCard.tsxs actually come directly from the StackScripts endpoint. The AppCard's title is just stackscript.label with some transformation done by getMarketplaceAppLabel.

I see now, adding an additional .replace('NodeJS', 'Node.js'); would fix the title in the Card.

I'm thinking that, rather than updating the hardcoded name on our end, we might want update the AppDetailsDrawer to show the StackScript label rather than the hardcoded title so that the marketplace team can update what the API returns and fix title issues without any code changes.
The changes in the description and guides still have to be done hard-coded so those changes look good to me.

I like this idea. To coordinate this update would I make my changes and wait for the marketplace team to make their changes before merging my PR?

bnussman-akamai · 2024-10-11T15:42:18Z

I see now, adding an additional .replace('NodeJS', 'Node.js'); would fix the title in the Card.

Right. I'm suggesting we just show the stackscrpt label rather than Cloud Manager's hardcoded name and ask the Marketplace team to update the label so that we won't need to do any client side replacement

To coordinate this update would I make my changes and wait for the marketplace team to make their changes before merging my PR?

I don't think the changes need to be super coordinated. It seems like there is already some inconsistency with NodeJS vs Node.js so I don't think it's a huge deal to coordinate

pmakode-akamai

Looks good!

I'm in favor of showing the StackScript label rather than the hardcoded name as well

mjac0bs

I see now, adding an additional .replace('NodeJS', 'Node.js'); would fix the title in the Card.

Right. I'm suggesting we just show the stackscrpt label rather than Cloud Manager's hardcoded name and ask the Marketplace team to update the label so that we won't need to do any client side replacement

Sounds good to me too - I like the thought of consistency with label everywhere, in the Marketplace team's control.
@carrillo-erik Something like this would show all the necessary changes in AppDetailDrawer and I imagine there will be test changes required too.

  const selectedApp = apps.find((app) => app.stackscript.id === stackScriptId);
  const displayLabel = selectedApp
    ? getMarketplaceAppLabel(selectedApp?.stackscript.label)
    : ''; // Use everywhere name was

To coordinate this update would I make my changes and wait for the marketplace team to make their changes before merging my PR?

I don't think the changes need to be super coordinated. It seems like there is already some inconsistency with NodeJS vs Node.js so I don't think it's a huge deal to coordinate

Agreed. I would reach out to @hmorris3293 or @tbaka-akamai internally to make sure they're aware of this change and see no issue with it, then we can revert this PR's current changes to name and they can finish the label updates on their end. We could clean up of all oneClickAppsv2.ts names in a separate PR, but I don't think it would be too much scope creep to do it in this one.

…/M3-7158

tbaka-akamai · 2024-10-15T14:25:22Z

howdy @mjac0bs updating the label is trivial on our end. We went ahead and made the change for Node.JS and Openlitespeed Node.JS on the backend. It appears to render correctly in the Cloud Manager.

carrillo-erik · 2024-10-15T14:28:31Z

howdy @mjac0bs updating the label is trivial on our end. We went ahead and made the change for Node.JS and Openlitespeed Node.JS on the backend. It appears to render correctly in the Cloud Manager.

@tbaka-akamai Thanks for the quick turnaround on this, there's only one minor change is that the JS after the period should be lower-case. The cards should read Node.js and OpenLiteSpeed Node.js.

tbaka-akamai · 2024-10-15T15:01:23Z

No problem @carrillo-erik. I have updated the casing on the labels.

carrillo-erik · 2024-10-15T17:10:09Z

@bnussman-akamai @mjac0bs
I've reverted the changes to the hardcoded name property but kept the changes to the summary, description, and related_guides. Additionally, @tbaka-akamai did some updates which fix the title for Node.js and OpenLiteSpeed Node.js in the AppSelectionCard component.

Next, I will complete the following steps.

Update the utility function getMarketplaceAppLabel() so that we won't need to do any client side replacement
as requested by @bnussman-akamai.
export const getMarketplaceAppLabel = (label: string) => decode(label);
Implement the changes and updates to tests in AppDetailDrawer as requested by @mjac0bs.

const selectedApp = apps.find((app) => app.stackscript.id === stackScriptId);
const displayLabel = selectedApp
? getMarketplaceAppLabel(selectedApp?.stackscript.label)
: ''; // Use everywhere name was

Clean up of all names in oneClickAppsv2.ts as requested by @mjac0bs.

However, I'd like some further clarification with this statement to better understand how to effectively close this PR:

then we can revert this PR's current changes to name and they can finish the label updates on their end.

mjac0bs · 2024-10-15T17:16:36Z

However, I'd like some further clarification with this statement to better understand how to effectively close this PR:

then we can revert this PR's current changes to name and they can finish the label updates on their end.

@carrillo-erik - you reverted the changes to the hardcoded name property and Travis has updated the labels, as per his comments above, so this part is done. 👍🏼

…/M3-7158

…in AppDetailDrawer

mjac0bs

✅ Node.js is capitalized correctly everywhere it is used.
✅ We are consistently using the stackscript label (with our standard manipulation via the util) rather than the hardcoded name. We still use the util because all OCA stackscripts contain some extra information in the label that we don't want to display to the user.
✅ We removed the hardcoded name from oneClickAppsv2.ts without regressions.
✅ Tests pass - CI failures were unrelated, though the support ticket one could have been resolved by merging in the latest develop

bnussman-akamai · 2024-10-16T18:23:45Z

packages/manager/cypress/e2e/core/oneClickApps/one-click-apps.spec.ts

+          const displayLabel = stackscript
+            ? getMarketplaceAppLabel(stackscript.label)
+            : '';


Can we move this below the if statement? No need to fallback to an empty string

Suggested change

const displayLabel = stackscript

? getMarketplaceAppLabel(stackscript.label)

: '';

const displayLabel = getMarketplaceAppLabel(stackscript.label);

Moving this line above the if statement would lead to a scope error for displayLabel as it is used in the error message that we throw on Lines 46-47.

You have to scroll far into the right to see it:
We expected that StackScript to be in the response for the Marketplace app named "${displayLabel}".

Right, but wouldn't displayLabel be an empty string if that error was thrown? If that's the case, we might aswell just remove that sentence from the error message

We could change the error message to:
Cloud Manager's fetch to GET /v4/linode/stackscripts did not receive a StackScript with ID ${stackscriptId}. We expected a StackScript to be in the response.
This would allow us to make the change you suggested, resolve the empty string, and the deal with the variable scope error.

I wrote the last comment before noticing you had edited your comment above. I removed the stackscript.label reference from the error message.

Nice. Looks great now!

bnussman-akamai · 2024-10-16T18:25:26Z

packages/manager/src/features/OneClickApps/types.ts

@@ -11,7 +11,6 @@ export interface OCA {
   */
  isNew?: boolean;
  logo_url: string;
-  name: string;


This is great 👏 🎉

Less hardcoded stuff === 😄

…/M3-7158

bnussman-akamai

Looks good pending the clean up in one-click-apps.spec.ts

jdamore-linode

Thanks @carrillo-erik! +1 to @bnussman-akamai's suggestion to move the const declaration but otherwise tests look (and run) great!

cypress · 2024-10-16T22:37:24Z

Cloud Manager E2E Run #6688

Run Properties: Passed #6688 • dda6bf987b: feat: [M3-7158] - Update NodeJS naming to Node.js for Marketplace (#11086)

Project	`Cloud Manager E2E`
Run status	`Passed #6688`
Run duration	`26m 48s`
Commit	`dda6bf987b: feat: [M3-7158] - Update NodeJS naming to Node.js for Marketplace (#11086)`
Committer	`carrillo-erik`
View all properties for this run ↗︎

Test results
Failures	`0`
Flaky	`4`
Pending	`2`
Skipped	`0`
Passing	`437`

feat: [M3-7158] - Update NodeJS naming to Node.js for Marketplace

b5098f0

carrillo-erik added the Marketplace label Oct 10, 2024

carrillo-erik self-assigned this Oct 10, 2024

carrillo-erik requested a review from a team as a code owner October 10, 2024 16:56

carrillo-erik requested review from mjac0bs and bnussman-akamai and removed request for a team October 10, 2024 16:56

pmakode-akamai approved these changes Oct 14, 2024

View reviewed changes

mjac0bs reviewed Oct 14, 2024

View reviewed changes

Merge branch 'develop' of https://github.com/linode/manager into feat…

02aaa98

…/M3-7158

Revert changes to hardcoded name in oneClickAppsv2.tsx

efbd5bd

Add changeset

86cd06b

carrillo-erik added 3 commits October 15, 2024 14:56

Merge branch 'develop' of https://github.com/linode/manager into feat…

c88c5bb

…/M3-7158

Update the logic to display label vs name

475f4a9

Update e2e test to use stackscript.label instead of app.name

d575373

carrillo-erik requested a review from a team as a code owner October 15, 2024 23:30

carrillo-erik requested review from cliu-akamai and removed request for a team October 15, 2024 23:30

carrillo-erik added 2 commits October 16, 2024 07:52

Revert changes to getMarketplaceAppLabel() and fix failing unit test …

abb8651

…in AppDetailDrawer

Remove name field from oneClickApps in favor of stackscript label

4aad11e

mjac0bs approved these changes Oct 16, 2024

View reviewed changes

bnussman-akamai reviewed Oct 16, 2024

View reviewed changes

Merge branch 'develop' of https://github.com/linode/manager into feat…

7a2497a

…/M3-7158

bnussman-akamai approved these changes Oct 16, 2024

View reviewed changes

jdamore-linode approved these changes Oct 16, 2024

View reviewed changes

PR feedback and merge latest from develop branch

30bfe77

carrillo-erik merged commit dda6bf9 into linode:develop Oct 16, 2024
21 of 23 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: [M3-7158] - Update NodeJS naming to Node.js for Marketplace #11086

feat: [M3-7158] - Update NodeJS naming to Node.js for Marketplace #11086

carrillo-erik commented Oct 10, 2024 •

edited

Loading

github-actions bot commented Oct 10, 2024 •

edited

Loading

carrillo-erik commented Oct 10, 2024

bnussman-akamai commented Oct 11, 2024

carrillo-erik commented Oct 11, 2024

bnussman-akamai commented Oct 11, 2024

pmakode-akamai left a comment •

edited

Loading

mjac0bs left a comment

tbaka-akamai commented Oct 15, 2024

carrillo-erik commented Oct 15, 2024 •

edited

Loading

tbaka-akamai commented Oct 15, 2024

carrillo-erik commented Oct 15, 2024

mjac0bs commented Oct 15, 2024

mjac0bs left a comment

bnussman-akamai Oct 16, 2024

carrillo-erik Oct 16, 2024

bnussman-akamai Oct 16, 2024 •

edited

Loading

carrillo-erik Oct 16, 2024

carrillo-erik Oct 16, 2024

bnussman-akamai Oct 16, 2024

bnussman-akamai Oct 16, 2024

bnussman-akamai left a comment

jdamore-linode left a comment

cypress bot commented Oct 16, 2024

feat: [M3-7158] - Update NodeJS naming to Node.js for Marketplace #11086

feat: [M3-7158] - Update NodeJS naming to Node.js for Marketplace #11086

Conversation

carrillo-erik commented Oct 10, 2024 • edited Loading

Description 📝

Changes 🔄

Preview 📷

How to test 🧪

Verification steps

As an Author I have considered 🤔

github-actions bot commented Oct 10, 2024 • edited Loading

carrillo-erik commented Oct 10, 2024

bnussman-akamai commented Oct 11, 2024

carrillo-erik commented Oct 11, 2024

bnussman-akamai commented Oct 11, 2024

pmakode-akamai left a comment • edited Loading

Choose a reason for hiding this comment

mjac0bs left a comment

Choose a reason for hiding this comment

tbaka-akamai commented Oct 15, 2024

carrillo-erik commented Oct 15, 2024 • edited Loading

tbaka-akamai commented Oct 15, 2024

carrillo-erik commented Oct 15, 2024

mjac0bs commented Oct 15, 2024

mjac0bs left a comment

Choose a reason for hiding this comment

bnussman-akamai Oct 16, 2024

Choose a reason for hiding this comment

carrillo-erik Oct 16, 2024

Choose a reason for hiding this comment

bnussman-akamai Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

carrillo-erik Oct 16, 2024

Choose a reason for hiding this comment

carrillo-erik Oct 16, 2024

Choose a reason for hiding this comment

bnussman-akamai Oct 16, 2024

Choose a reason for hiding this comment

bnussman-akamai Oct 16, 2024

Choose a reason for hiding this comment

bnussman-akamai left a comment

Choose a reason for hiding this comment

jdamore-linode left a comment

Choose a reason for hiding this comment

cypress bot commented Oct 16, 2024

Cloud Manager E2E Run #6688

carrillo-erik commented Oct 10, 2024 •

edited

Loading

github-actions bot commented Oct 10, 2024 •

edited

Loading

pmakode-akamai left a comment •

edited

Loading

carrillo-erik commented Oct 15, 2024 •

edited

Loading

bnussman-akamai Oct 16, 2024 •

edited

Loading