-
Notifications
You must be signed in to change notification settings - Fork 85
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! What's next?Please work through the following steps to get your changes ready for engineering review: 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. 🔘 Let us know that your PR is ready for review:Who will review my changes?This repository is currently maintained by Where can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:
When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
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? |
@shadinaif @OmarIthawi Sorry for the delay here. Is this PR still needed? @shadinaif If so, could you please resolve merge conflicts? Once the build is green we can have someone from Axim engineering merge the PR. |
yes it's still needed @itsjeyd |
Got it, thanks @OmarIthawi. @shadinaif Over to you for now. Let me know when you're done resolving merge conflicts here. |
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