-
Notifications
You must be signed in to change notification settings - Fork 81
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
fix: Data too long for column 'resource_id' #435
base: master
Are you sure you want to change the base?
fix: Data too long for column 'resource_id' #435
Conversation
Thanks for the pull request, @shadinaif! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
For your review @OmarIthawi, please |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #435 +/- ##
==========================================
+ Coverage 97.83% 97.85% +0.01%
==========================================
Files 77 78 +1
Lines 6710 6709 -1
==========================================
Hits 6565 6565
+ Misses 145 144 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Thanks @shadinaif please update the following: |
bb1ad9b
to
454346f
Compare
please check now @OmarIthawi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Shadi. One more thing please.
454346f
to
ab4df99
Compare
ab4df99
to
c5851e6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Thanks @shadinaif!!
Hi @OmarIthawi and @shadinaif! Is this pull request still needed? |
Yes @mphilbrick211, we needed it to get merged but it wasn't triaged since Jan 25th. @shadinaif there's a new changelog update that conflicts with this pull request, kindly rebase and fix it. |
c0f4d1f
to
f6fbae4
Compare
Rebased. But codecov is failing for no reason ¯\(ツ)/¯ |
Hi @shadinaif and @OmarIthawi! Can this be closed? |
when the course id is a little bit long, the lti xblock id becomes too long for resource_id to handle
f6fbae4
to
df5fd9b
Compare
Thanks @shadinaif. @mphilbrick211, this is now ready for merging. I don't have permissions though. Could you please help? |
when the course id is a little bit long, the lti xblock id becomes too long for resource_id to handle
To reproduce:
course-v1:Public+TEST_IT_01+TEST_IT_01_Dec_2023
lti_consumer
in Advanced settings for the course1.3
we tried that on the latest https://sandbox.openedx.org