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.

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

feat(3636): track reasons for choosing cloud provider #3857

Closed
wants to merge 12 commits into from

Conversation

golebu2020
Copy link
Contributor

Screenshot 2024-09-25 at 10 45 29 AM

app/utils/number.ts Fixed Show fixed Hide fixed
Copy link
Collaborator

@junminahn junminahn left a comment

Choose a reason for hiding this comment

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

Looks great! I just left one comment.

Quick question: Have you tested the feature with old data, such as from your local environment or live environments?

@@ -193,7 +193,14 @@ export default publicCloudProductRequest(({ pathParams, queryParams, session, ro

return (
<div>
<PublicCloudBillingInfo product={publicSnap.currentRequest.decisionData} className="mb-2" />
<PublicCloudBillingInfo
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems you're adding these two fields to align with the type configuration. Instead of adding data, let's omit these two fields from the Product type in PublicCloudBillingInfo.

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've tested the feature from my local environment and it works as expected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems you're adding these two fields to align with the type configuration. Instead of adding data, let's omit these two fields from the Product type in PublicCloudBillingInfo.

okay

Copy link
Collaborator

Choose a reason for hiding this comment

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

I've tested the feature from my local environment and it works as expected.

Did your local environment had a old public cloud data?

Copy link
Contributor Author

@golebu2020 golebu2020 Sep 26, 2024

Choose a reason for hiding this comment

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

No I don't. The sign MoU feature didn't allow me to go past the level of approving the product.

@@ -20,3 +21,12 @@ export const getRandomUser = () => {
createdAt: new Date(),
};
};
export const getRandomCloudProviderSelectionReasons = () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I see duplicate function getRandomCloudProviderSelectionReasons from above.

const randomNumberOfReasons = getRandomNumberOptimally(1, reasonForSelectingCloudProviderArray.length);
return faker.helpers.arrayElements(reasonForSelectingCloudProviderArray, randomNumberOfReasons);
};
export const getRandomProviderReasonsNote = () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I see duplicate function getRandomProviderReasonsNote from above.

@golebu2020
Copy link
Contributor Author

Looks great! I just left one comment.

Quick question: Have you tested the feature with old data, such as from your local environment or live environments?

I've tested the feature from my local environment and it works as expected.

.bin/copy-db.sh Outdated
@@ -28,4 +28,5 @@ echo "Most recent file: $recent_file"

# Copy the most recent back file to unarchive into the sandbox database
oc cp "$pod_name:backup/$recent_file" "$tmp_dir/$recent_file"
echo "pltsvc"
Copy link
Collaborator

Choose a reason for hiding this comment

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

please revert this.

.tool-versions Outdated
@@ -7,3 +7,4 @@ helm 3.16.1
shfmt 3.9.0
act 0.2.67
jq 1.7.1
database-tools 0.8.1
Copy link
Collaborator

Choose a reason for hiding this comment

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

please revert 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.

Did already

reviewMou: boolean;
};
},
'providerSelectionReasons' | 'providerSelectionReasonsNote'
Copy link
Collaborator

Choose a reason for hiding this comment

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

try revert this to see if you still have issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay

@@ -12,5 +12,5 @@ export type Product = Omit<
};
};
}>,
'updatedAt'
'updatedAt' | 'providerSelectionReasons' | 'providerSelectionReasonsNote'
Copy link
Collaborator

Choose a reason for hiding this comment

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

try revert this to see if you still have issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay

export const up = async (db, client) => {
await db
.collection('PublicCloudRequestedProject')
.updateMany({ providerSelectionReasonsNote: null }, { $set: { providerSelectionReasonsNote: ' ' } });
Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. let's merge these two files with Promise.all.
  2. in the filter clause, check if the field exists as well
  • $exists: false OR null

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay

@golebu2020 golebu2020 force-pushed the feat/3636-A branch 2 times, most recently from 5cca765 to f9afa9b Compare September 27, 2024 17:22
Copy link

sonarcloud bot commented Sep 27, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
4.4% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

@golebu2020 golebu2020 closed this Sep 27, 2024
@golebu2020 golebu2020 deleted the feat/3636-A branch September 27, 2024 18:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants