# Intros Code Review

## Jira Ticket Answers
**What areas of the server codebase did you find the most confusing? Where is the greatest potential for improving readability?**
At a micro-level (file/function-level), the server is not very confusing. The most confusing part for me was the macro-level patterns that the server implements. That being said, it didn't take long for me to understand them and it may just be a function of me coming from an ExpressJS background. We could improve readability by adding READMEs at the folder level to describe the contents and function of each folder.

**What areas of the server codebase did you find overly repetitive? Where is the greatest potential to simplify the code through modularization?**
The router is essentially flat in structure. I really like this since it allows for quick development and onboarding, but it means that we end up repeating a lot of the handler definitions. We could simplify that by creating a more hierarchical structure. We can also create classes for each resource to interact with the database. An ORM would be helpful here as well.

**What areas of the UI codebase did you find the most confusing? Where is the greatest potential for improving readability?**
The natively implemented intros/admin nested routes is a little confusing to me. I haven't looked into alternative solutions yet, but I suspect that we could leverage a prebuilt solution that people are already familiar with.

**What areas of the UI codebase did you find overly repetitive? Where is the greatest potential to simplify the code through modularization?**
Similar to the backend, we could abstract API resources out to their own classes that inherit from a base API class. This would take away the repeated code that I describe below. It also sets us up for a Python SDK that Intros could provide one day.

Are there any are**as of our Product ↔︎ Engineering pipeline that seem overly bureaucratized? Should we simplify this process anywhere?**
Without doing any development work so far, I don't think I can provide any meaningful feedback here. That being said, it seems pretty efficient and decoupled(teams aren't getting in the way of each other) which I think is a good sign.

**Are there any areas of our code release procedure that seem overly manual? Where should we automate?**
I don't have a deep understanding of the code release procedure that we currently follow, so it's tough to give any feedback here. I do think that the DevOps improvments that Hamza will implement will streamline some other things like keeping mindumps, environment variables, etc in sync.

**What are 1-3 ways in which you would recommend we establish healthy communication and culture on the team?**
In my opinion, the most successful engineering teams are ones where all of the engineers are happiest. Engineers are usually happiest when they feel valued, and able to contribe meaningfully and efficiently to the team. To achieve this, we should cultivate a culture that allows developers to implement features in their own way, but still within a framework that makes our product and codebase cohesive. Micromanagement in coding style (and other aspects) can really slow down momentum and moral.

**Are there any other changes you would like to see within the realm of engineering?**
Nothing else at the moment.

## Backend
**Server.py**
- server.py seems a little cluttered. A nice abstraction would be separating the router out into its own file, then importing that single file in server.py. We've already talked about making the router hierarchical.
- app.listen should be fed an environment variable. This should help with hosting since some services are opinionated in their port

**Logging**
- Can we create a transport for the logs to display in the terminal when running locally? Not sure if that is industry standard or just my preference.
- Are we storing logs somewhere other than the local folder? (Ideally in a db like ElasticSearch that allows for easy indexing/search). If not, I think we should do that. When we move to a multi-server architecture, it’s going to get more complex to look through server logs. On top of that, I think we will lose all of the logs when we deploy (swapping containers out).
- Our choice here should also be made with compliance/audits in mind.

**Database**
- I don't think we are storing timestamps on any records. This is probably something we should do in case future UI specs require us to show a createdAt or last editted date. We can consider other metadata to store as well.
- The table names are all singular (“user”, “groupEvent”, etc). Some would argue that standard REST practice says to name these plurally, but this ins’t a huge deal.
- We've already talked about this, but I think we should find a way to remove the test data from user records (test, altEmail, etc). This is too closely coupled with the testing suite, it will cause us to store unnecessary amounts of data, and It will also become confusing for external API users.

**General Thoughts**
- We are currently implementing our own auth service which may work in the short term, but long term this will almost certainly cost more to maintain, will not scale as nicely as we increase the number of authentication providers, and is generally less secure(I would bet on other companies dedicated to auth doing a better job with auth than us). AWS Cognito, Firebase, Okta, etc are all popular options here.
- It looks like we are sending bare email/passwords strings as it is.
- Currently we only require admins to sign in, how will the current situation scale with user accounts(the answer is probably passwordless here)?
- I love the idea of an ORM on top of SQL that Hamza mentioned. This really cleans up the code. I don't enough Python or SQL experience to reccommend a specific ORM.
- Is it secure to check in `server/core/api_clients/service-account.json` into git?
- Can we delete `server/these`?
- I’ve noticed a lot of unused imports. Can we use a linter to catch these for us? Maybe make linting part of the CI pipeline?
- We can combine the 2 mixpanel functions into 1 pass a member/admin boolean.
- The models in `server/models/*` make database calls with frequently repeated code. There is a spectrum on which we could modularize this code. At a minimum, I think it makes sense to do things like abstract out the table names into variables like `userTable = ‘meetSunday.user’`. That will make the migration to a db called “intros” (amongst other changes) a little easier. On the other end of the spectrum, we could have a base class that acts as a factory function to define CRUD methods for a given resource to interact with the database. We could extend that class with resource-specific methods. If we moved to an ORM, the high-level idea here would still apply.
- Define constants for events that we track. This will keep things consistent and avoid any misspellings that can lead to corrupt analytics.
- I’ve seen some repos with READMEs in each folder that describe the contents of the folder. “This is where all of the database models are. They interact with the data layer…”. This might be a good thing to implement for future hires to reduce ramp up time.

## Frontend
**Larger Lift Suggestions**
- [Storybook](https://storybook.js.org/docs/react/writing-tests/introduction) for testing?
- [Tailwind](https://tailwindcss.com/) for css? Would simplify site-wide theming, allow us to remove custom utility classes, makes responsive design easier to achieve.
- Server side rendering? SEO, performance, other benefits. [Next.js](https://nextjs.org/) is a common option for this. Great integration with Vercel (vercel maintains next.js).

**More granular changes**
- We can clean up App.js by defining routes in a separate router file. We should be able to programmatically define these.
- The intros component router effectively implements nested routes. Should we implement that? I think it can minimally be cleaned up while still keeping the custom routing solution.
- Can we delete the top-level package-lock.json and README?
- All of the images in `/public` and all of the videos in `/public/videos` should be nested under `public/assets/images` and `public/assets/videos`
- We should abstract away API calls into classes organized by resource type. For example, an API call that looks like this:

return RestApiClient.request('/admin/question-bank/join', {
      method: HttpMethodEnum.POST,
      body: {
        ...params,
        pw: process.env.REACT_APP_ADMIN_API_PW
      }
    }).pipe(…)

Would look like this:

return admin.questionBank.join.pipe(…)

- There are some unused imports like the backend. I think it makes sense to configure prettier to catch those and add linting to our CI pipeline
- Defining `baseURL` In `src/models/api/rest/restApiClientModel.ts` should be done through an env var like `REACT_APP_INTROS_API_BASE_URL`. That will scale a little nicer as we create new environments, will enable us to hit different backends from the same type of environment (maybe we have different servers in different parts of the world?), and will enable us to change it to a IP address to test locally within a local network. Same for web sockets connection.
- Can we inject environment variables into a docker container? It looks like the app is being deployed inside of a Docker container and served via nginx- can we run that locally via docker compose and pass in some environment variables there? Not sure if this will require a rebuild everytime since I think env vars are set at build time. The benefit here is that we can mimic the hosted environment and simplify some of scripts in `package.json`.
``"local": "cross-env REACT_APP_MODE=local REACT_APP_UI=http://localhost:3000 react-scripts start"`` becomes `"local": react-script start` because all of the env vars are injected from docker-compose.