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.

Sign up for GitHub

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

Jump to bottom

Add College of Medicine integration #46

Merged
merged 18 commits into from
Nov 29, 2023
Merged

Add College of Medicine integration #46

merged 18 commits into from
Nov 29, 2023

Conversation

ddecollo
Copy link
Contributor

Fixes #36

@ddecollo ddecollo requested a review from ajkiessl July 27, 2023 21:20
ajkiessl
ajkiessl requested changes Jul 31, 2023
View reviewed changes
Copy link
Contributor

@ajkiessl ajkiessl left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I think we are actually are pretty close to having everything tested. I added a few things to change. We should also add a test in the models/faculty_spec.rb to test the has_many association with efforts and qualities.

spec/models/com_effort_spec.rb Outdated Show resolved Hide resolved
spec/models/com_quality_spec.rb Outdated Show resolved Hide resolved
spec/importers/com_data/com_effort_xml_builder_spec.rb Outdated Show resolved Hide resolved
app/importers/activity_insight/get_user_data.rb Outdated Show resolved Hide resolved
@ajkiessl
Copy link
Contributor

ajkiessl commented Jul 31, 2023 •
edited
Loading

Also, this needs rebased with main and might (if it wasn't already) need linted with rubocop.

Edit: Actually scratch the rubocop. I don't think it is fully set up yet.

@ddecollo ddecollo requested a review from ajkiessl August 2, 2023 13:40
ajkiessl
ajkiessl requested changes Aug 2, 2023
View reviewed changes
Copy link
Contributor

@ajkiessl ajkiessl left a comment •
edited
Loading

Choose a reason for hiding this comment

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

The changes you made look good, but I noticed an issue below 👇 . Also, we'll need to merge main into this and resolve conflicts. CI is still failing but I'm not too sure why. Hopefully, one of these changes will fix it.

db/migrate/20230705195916_create_com_efforts.rb Outdated Show resolved Hide resolved
@ddecollo ddecollo requested a review from ajkiessl August 2, 2023 20:23
@ajkiessl
Copy link
Contributor

@ddecollo @Smullz622 Just a reminder, we should try to get this deployed to production sometime before the end of the internship. I'll try to get this reviewed today or next week.

ajkiessl
ajkiessl approved these changes Nov 20, 2023
View reviewed changes
Copy link
Contributor

@ajkiessl ajkiessl left a comment

Choose a reason for hiding this comment

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

This looks good. I think Sarah ran this a couple times recently for Nicole to present to some stakeholders. If it ran properly, then I think it's good to merge and deploy this to production. We should then do one last import with beta and check with Nicole that the data looks good before running an import with production.

@ddecollo ddecollo merged commit 6734f6c into main Nov 29, 2023
4 checks passed
@ddecollo ddecollo deleted the 36_com_data_import branch November 29, 2023 20:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ajkiessl ajkiessl ajkiessl approved these changes

@Smullz622 Smullz622 Awaiting requested review from Smullz622

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

College of Medicine data import
2 participants
@ddecollo @ajkiessl