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

[Add]: set up flow to add lms to course #41

Merged
merged 34 commits into from
Apr 28, 2024

Conversation

Zzz212zzZ
Copy link

General Info

Changes

Implement the POST api/v1/courses/{course_id}/lms, which creates one record in the course_to_lms table.

Testing

  • Test if it raises exception when course_id and lms_id are invalid;
  • Test if it raises exception when external_course_id is missing.
  • Test if it raises exception if this bind between such this course and lms exits before.
  • Test if such a target record could be successfully created in the course_to_lms table.

Documentation

The related tables in this implementation part in our E-R diagram are shown below.
ER Diagram

Checklist

  • Name of branch corresponds to story

Copy link
Member

@Connor-Bernard Connor-Bernard left a comment

Choose a reason for hiding this comment

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

#39 also has these changes. It would be great if you could remove this pr's code from the other one and resolve the comments I left on that one :)

@Zzz212zzZ
Copy link
Author

Zzz212zzZ commented Apr 24, 2024

#39 also has these changes. It would be great if you could remove this pr's code from the other one and resolve the comments I left on that one :)

Hi Connor, thanks for your comments! I've already made changes based on your comments on #39. Sorry for the inconvenience caused by the huge pr. Please let me know if I should make further modification on this feature branch, thanks!

Copy link

@armandofox armandofox left a comment

Choose a reason for hiding this comment

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

generally OK, made a couple of minor comments, will leave final merge authority within the team since this is the golden repo, de facto.

app/controllers/api/v1/lmss_controller.rb Outdated Show resolved Hide resolved
Connor-Bernard
Connor-Bernard previously approved these changes Apr 26, 2024
Copy link
Member

@Connor-Bernard Connor-Bernard left a comment

Choose a reason for hiding this comment

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

Couple of comments, but overall looks good.

app/controllers/api/v1/lmss_controller.rb Show resolved Hide resolved
app/controllers/api/v1/lmss_controller.rb Outdated Show resolved Hide resolved
app/controllers/api/v1/lmss_controller.rb Outdated Show resolved Hide resolved
app/helpers/canvas_validation_helper.rb Outdated Show resolved Hide resolved
Copy link
Member

@Connor-Bernard Connor-Bernard left a comment

Choose a reason for hiding this comment

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

LGTM

@Zzz212zzZ Zzz212zzZ merged commit cd1ed11 into main Apr 28, 2024
6 checks passed
@Zzz212zzZ Zzz212zzZ deleted the 187434223-set-up-flow-to-add-lms-to-course branch April 28, 2024 04:30
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.

4 participants