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

Added Applied Economics Minor #773

Merged
merged 6 commits into from
Feb 13, 2023
Merged

Added Applied Economics Minor #773

merged 6 commits into from
Feb 13, 2023

Conversation

mirandayu131
Copy link
Collaborator

@mirandayu131 mirandayu131 commented Feb 8, 2023

Summary
Added the Business minor for Applied Economics in the SC Johnson College of Business.

Information on the requirements is sourced from the following:
[https://business.cornell.edu/programs/undergraduate/minors/applied-economics/]

Test Plan
Test AE minor!

@mirandayu131 mirandayu131 requested a review from a team as a code owner February 8, 2023 04:17
@dti-github-bot
Copy link
Member

dti-github-bot commented Feb 8, 2023

[diff-counting] Significant lines: 66.

@zachary-kent
Copy link
Collaborator

@jerrry1123 @rohanmaheshwari430 might want to review this for dev portfolio.

@github-actions
Copy link
Contributor

github-actions bot commented Feb 8, 2023

Visit the preview URL for this PR (updated for commit a27dd8c):

https://cornelldti-courseplan-dev--pr773-aem-minor-99urvnts.web.app

(expires Fri, 10 Mar 2023 18:06:42 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: 6d4e0437c4559ed895272bbd63991394f1e0e933

@noschiff
Copy link
Member

noschiff commented Feb 8, 2023

make sure the minor is added to the req list!

@@ -375,6 +376,12 @@ const json: RequirementsJson = {
},
},
minor: {
AEM: {
name: 'Applied Economics and Management',
schools: ['AG', 'BU'],
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't every college except dyson be allowed to do the minor?

Copy link
Member

Choose a reason for hiding this comment

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

@zachary0kent am I wrong about how this works

Copy link
Contributor

Choose a reason for hiding this comment

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

i know that some colleges like engineering ahve their own version of their AEM minor

Copy link
Collaborator

@zachary-kent zachary-kent Feb 13, 2023

Choose a reason for hiding this comment

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

@noschiff I believe this refers to which college offers the minor, not those eligible.

Copy link
Member

Choose a reason for hiding this comment

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

where do we store which colleges are eligible? @zachary0kent

Copy link
Collaborator

@zachary-kent zachary-kent Feb 13, 2023

Choose a reason for hiding this comment

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

I don't believe we do? I'm not aware of such restrictions typically existing anyways.

@zachary-kent
Copy link
Collaborator

Remember to run npm run req-gen!

Copy link
Member

@noschiff noschiff left a comment

Choose a reason for hiding this comment

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

Can we name this "Applied Economics" instead of AEM? The minor is officially applied economics, whereas AEM usually refers to the Dyson major (M for & Management).

@mirandayu131 mirandayu131 changed the title Added AEM Minor Added Applied Economics Minor Feb 8, 2023
@noschiff
Copy link
Member

noschiff commented Feb 8, 2023

@mirandayu131 can you add more information to the PR overview? It'd be good to have a link to the source of requirements for the reviewer to reference, as well as a test plan. The test plan for adding a major is usually pretty brief though :)

Copy link
Member

@noschiff noschiff 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'll leave the approval + testing to one of the devs for their portfolio :)

@jerrry1123
Copy link
Contributor

I did the testing plan and all the requirements look right, I just think that it might be better to rename the minor to non-engineering or something like that to differentiate this minor from the engineering business minor.

Copy link
Contributor

@jerrry1123 jerrry1123 left a comment

Choose a reason for hiding this comment

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

I did the testing plan and all the requirements look right, I just think that it might be better to rename the minor to non-engineering or something like that to differentiate this minor from the engineering business minor.

@@ -375,6 +376,12 @@ const json: RequirementsJson = {
},
},
minor: {
AEM: {
name: 'Applied Economics and Management',
schools: ['AG', 'BU'],
Copy link
Contributor

Choose a reason for hiding this comment

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

i know that some colleges like engineering ahve their own version of their AEM minor

@mirandayu131 mirandayu131 merged commit c99d409 into master Feb 13, 2023
@mirandayu131 mirandayu131 deleted the aem_minor branch February 13, 2023 22:50
@noschiff noschiff added the req data Improvements or additions to requirements data label Feb 13, 2023
@noschiff noschiff mentioned this pull request Apr 26, 2023
22 tasks
This was referenced May 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
req data Improvements or additions to requirements data
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants