Removes unused code, updates dependencies, new README, and other items #86
Conversation
![datadog-datadog-prod-us1[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
| {"error": "Discount service error", "status": -1}) | ||
| err.status_code = 500 | ||
| return err | ||
| raise Exception("Discount service error") |
There was a problem hiding this comment.
⚪ Code Quality Violation
Exception is too generic (...read more)
Do not raise Exception and BaseException. These are too generic. Having generic exceptions makes it difficult to differentiate errors in a program. Use a specific exception, for example, ValueError, or create your own instead of using generic ones.
Learn More
| import type { LineItem } from '@commerce/types/cart' | ||
|
|
||
| const countItem = (count: number, item: LineItem) => count + item.quantity | ||
| const countItem = (count: number, item: any) => count + item.quantity |
There was a problem hiding this comment.
⚪ Code Quality Violation
Unexpected any. Specify a different type. (...read more)
Do not use the any type, as it is too broad and can lead to unexpected behavior.
| InferGetServerSidePropsType<typeof getServerSideProps>) { | ||
| console.log(taxons) | ||
| }: InferGetServerSidePropsType<typeof getServerSideProps>) { | ||
| console.log(pages) |
There was a problem hiding this comment.
🟠 Code Quality Violation
Unexpected console statement. (...read more)
Debugging with console is not considered a bad practice, but it's easy to forget about console statements and leave them in production code. There is no need to pollute production builds with debugging statements.
| let products: Product[] = await fetch(`${baseUrl}/products`) | ||
| .then((res) => res.json()) | ||
| .catch((error) => { | ||
| console.error(error) |
There was a problem hiding this comment.
🟠 Code Quality Violation
Unexpected console statement. (...read more)
Debugging with console is not considered a bad practice, but it's easy to forget about console statements and leave them in production code. There is no need to pollute production builds with debugging statements.
| const headers = { | ||
| 'X-Throw-Error': `${flag}`, | ||
| 'X-Error-Rate': process.env.NEXT_PUBLIC_ADS_ERROR_RATE || '0.25', | ||
| } | ||
|
|
||
| try { | ||
| console.log('ads path', adsPath) |
There was a problem hiding this comment.
🟠 Code Quality Violation
Unexpected console statement. (...read more)
Debugging with console is not considered a bad practice, but it's easy to forget about console statements and leave them in production code. There is no need to pollute production builds with debugging statements.
| }) | ||
| .catch((e) => { | ||
| console.error(e.message) | ||
| console.error('An error occurred while fetching the discount code:', e) |
There was a problem hiding this comment.
🟠 Code Quality Violation
Unexpected console statement. (...read more)
Debugging with console is not considered a bad practice, but it's easy to forget about console statements and leave them in production code. There is no need to pollute production builds with debugging statements.
| @@ -38,33 +37,33 @@ def initialize_database(app, db): | |||
| discount_type = DiscountType(words.get_random(), | |||
| 'price * %f' % random.random(), | |||
| Influencer(names.get_full_name())) | |||
| discount_name = words.get_random(random.randint(2,4)) | |||
| discount_name = words.get_random(random.randint(2, 4)) | |||
There was a problem hiding this comment.
🔴 Code Vulnerability
do not use random (...read more)
Make sure to use values that are actually random. The random module in Python should generally not be used and replaced with the secrets module, as noted in the official Python documentation.
Learn More
| discount = Discount(discount_name, | ||
| words.get_random().upper(), | ||
| random.randrange(1,100) * random.random(), | ||
| random.randrange(1, 100) * random.random(), |
There was a problem hiding this comment.
🔴 Code Vulnerability
do not use random (...read more)
Make sure to use values that are actually random. The random module in Python should generally not be used and replaced with the secrets module, as noted in the official Python documentation.
Learn More
| "node_modules/bootstrap": { | ||
| "version": "4.6.1", | ||
| "resolved": "https://registry.npmjs.org/bootstrap/-/bootstrap-4.6.1.tgz", | ||
| "integrity": "sha512-0dj+VgI9Ecom+rvvpNZ4MUZJz8dcX7WCX+eTID9+/8HgOkv3dsRzi8BGeZJCQU6flWQVYxwTQnEZFrmJSEO7og==", | ||
| "license": "MIT", | ||
| "funding": { | ||
| "type": "opencollective", | ||
| "url": "https://opencollective.com/bootstrap" | ||
| }, | ||
| "peerDependencies": { | ||
| "jquery": "1.9.1 - 3", | ||
| "popper.js": "^1.16.1" | ||
| } | ||
| }, |
There was a problem hiding this comment.
🟠 Library Vulnerability
bootstrap → 4.6.1
Bootstrap Cross-Site Scripting (XSS) vulnerability (...read more)
| Faker==18.3.2 | ||
| Werkzeug==2.2.2 |
There was a problem hiding this comment.
🔴 Library Vulnerability
werkzeug → 2.2.2
High resource usage when parsing multipart form data with many fields (...read more)
| const pages: Page[] = await fetch(`${baseUrl}/pages`) | ||
| .then((res) => res.json()) | ||
| .catch((error) => { | ||
| console.error(error) |
There was a problem hiding this comment.
🟠 Code Quality Violation
Unexpected console statement. (...read more)
Debugging with console is not considered a bad practice, but it's easy to forget about console statements and leave them in production code. There is no need to pollute production builds with debugging statements.
.env.template
Outdated
| # called in client-side fetch calls from frontend service (no need to include the domain) | ||
| NEXT_PUBLIC_ADS_ROUTE=/services/ads | ||
| NEXT_PUBLIC_DISCOUNTS_ROUTE=/services/discounts | ||
| NEXT_PUBLIC_DBM_ROUTE="/services/dbm" |
There was a problem hiding this comment.
Is there a particular reason we have double quotes here, no quotes in lines 35 and 36, and single quotes in 29 and 30?
There was a problem hiding this comment.
Nope, that's a mistake, thank you for catching.
| DBM_PORT=7595 | ||
| DD_API_KEY= | ||
| DD_APP_KEY= | ||
| DATADOG_API_KEY= |
There was a problem hiding this comment.
This is specifically for the CI tool, right? If so, can we just add a comment about that?
| @@ -1,233 +1,165 @@ | |||
| # Storedog | |||
|
|
|||
| This a dockerized [Spree Commerce](https://spreecommerce.org) application consumed by a NextJS frontend. | |||
| Storedog is a Dockerized e-commerce site used primarily in labs run at [learn.datadoghq.com](https://learn.datadoghq.com). It consists of multiple services: | |||
There was a problem hiding this comment.
| Storedog is a Dockerized e-commerce site used primarily in labs run at [learn.datadoghq.com](https://learn.datadoghq.com). It consists of multiple services: | |
| Storedog is a Dockerized e-commerce site used primarily in labs run at [learn.datadoghq.com](https://learn.datadoghq.com). It consists of multiple services: |
Can we add something here to point readers to the "Breakdown of Services" section for more information?
There was a problem hiding this comment.
Alternatively, do you think maybe that info should really live in READMEs in each service's directory?
| ``` | ||
|
|
||
| Also update the Datadog agent service definition to include the following environment variable: |
There was a problem hiding this comment.
Do you also need to turn on the frontend feature flag? When does that need to be turned on?
| @@ -1,103 +0,0 @@ | |||
| import { FC, useEffect, useState, useCallback } from 'react' | |||
There was a problem hiding this comment.
If we can leave the login modal in the repo, I'd appreciate it. RUM masks elements with password attributes, which is useful to show to users. Even if we strip out the actual login auth stuff, the component itself is useful.
| @@ -1,52 +0,0 @@ | |||
| import { useEffect, useState } from 'react' | |||
There was a problem hiding this comment.
Can we leave the Search stuff in? It's used in some RUM courses and updating the Storedog image would then require either re-building a Search function or rewriting significant portions of the course (and making the learner do a lot more time-consuming clicks on the Storedog site)
| @@ -6,8 +6,8 @@ import { codeStash } from 'code-stash' | |||
| import config from '../../featureFlags.config.json' | |||
|
|
|||
| export async function getServerSideProps(context: GetServerSidePropsContext) { | |||
| const baseUrl = process.env.NEXT_PUBLIC_FRONTEND_URL | |||
| ? `${process.env.NEXT_PUBLIC_FRONTEND_URL}/api` | |||
| const baseUrl = process.env.NEXT_PUBLIC_FRONTEND_API_ROUTE | |||
There was a problem hiding this comment.
If we're checking for NEXT_PUBLIC_FRONTEND_API_ROUTE, why not just set the value to whatever/api in the root .env file? From what I can tell, we're always appending /api whenever that variable is used. Then you wouldn't need the ternary?
| @@ -1,4 +1,3 @@ | |||
| load_module "modules/ngx_http_geoip2_module.so"; | |||
There was a problem hiding this comment.
Why are we removing this?
There was a problem hiding this comment.
It never worked and I'm cutting dead weight
| @@ -3,7 +3,7 @@ import { Page } from '@customTypes/page' | |||
|
|
|||
| const SPREE_URL_SERVERSIDE = process.env.NEXT_PUBLIC_SPREE_API_HOST | |||
| ? `${process.env.NEXT_PUBLIC_SPREE_API_HOST}/api/v2` | |||
| : 'http://web:4000/api/v2' | |||
| : 'http://nginx/services/backend/api/v2' | |||
There was a problem hiding this comment.
If NEXT_PUBLIC_SPREE_API_HOST is http://nginx/services/backend, why are we checking for the variable to be set?
There was a problem hiding this comment.
I'm leaving things open for flexibility, some people may want to do different types of deploys at some point. I'm providing fallbacks just in case they aren't set.
| imageData.attributes.original_url | ||
| }`, | ||
| url: `${ | ||
| process.env.NEXT_PUBLIC_SPREE_API_HOST || 'http://backend:4000' |
There was a problem hiding this comment.
I'm not sure I understand what http://backend:4000 is here. When would that URL be used? Can it always be used?
There was a problem hiding this comment.
Yes, when running in a containerized environment (which is the default for Storedog) and using the docker network, this will refer to the backend service at port 4000.
There was a problem hiding this comment.
GH decided to not submit my comments b/c this PR is so big (I'm guessing), so I'll summarize here:
- Please keep the Login component. It's useful for exhibiting Session Replay's masking behavior on HTML elements with
passwordattributes. Don't need the API/actual login functionality, though. Maybe some dummy functionality with some hard-coded login creds. - Maybe also consider leaving the wishlist functionality in there. I think it would be useful for Product Analytics courses (e.g. "How many customers buy items after adding it to their wishlist?" and other customer journeys aside from simply "add item, add item, checkout")
- There's a lot of info in the README about each service ... should that info just live in the appropriate service READMEs instead? (Maybe for a future PR)
- Is there a reason to not just set the NEXT_PUBLIC_FRONTEND_API_ROUTE value with the
/adspath already appended? We do a lot of checking for NEXT_PUBLIC_FRONTEND_API_ROUTE with a ternary but from what I can tell, we only use${NEXT_PUBLIC_FRONTEND_API_ROUTE}/api. Then we could remove the ternary operators. Maybe I'm misunderstanding how/when the variables are used, though.
A lot going on here:
tldr: In an effort to gain better understanding of the most important services, this removes a lot of unused features, tightens up in a lot of areas, and updates a lot of documentation.
general
storedog_db.env.templatefilenginx
frontend
backend
puppeteer