-
Notifications
You must be signed in to change notification settings - Fork 16
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
feat: api migration #213
feat: api migration #213
Conversation
New endpoints: Delete account api/user/v1/accounts/deactivate_logout/ Dashboard Courses api/mobile/v3/users/{USERNAME}/course_enrollments Add Comment / Response api/discussion/v1/comments/ Discovery And Search api/courses/v1/courses/ Get Course Details api/mobile/v3/course_info/{COURSE_ID}/info Get Course Structure api/mobile/v3/course_info/blocks/
Thanks for the pull request, @volodymyr-chekyrta! 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. |
Rename DashCourse -> DashboardCourse
@miankhalid Is @saeedbashir available from Axinite side to review PRs or maybe @shafqat-muneer could do it? Thank you |
@rnr Sure, I will review from Axinite side. |
This endpoint is giving
|
Got the same feedback on the Android PR. Can you make sure you've got the latest changes from upstream? |
@volodymyr-chekyrta Our build pipeline is currently under code freeze, so, new changes might not have been deployed to our stage and prod environments |
@shafqat-muneer hi! |
Something weird is going on as only this API isn’t working on our end while the Enrollments v3 API ( our backend team is looking into this issue. in the meanwhile, @volodymyr-chekyrta do you know of any waffle flags or configurations that need to be done on the discovery service’s side for this API to work? |
Got it, thank you! |
@saeedbashir I've fixed the issue. Can you please retest it? |
@volodymyr-chekyrta I'm still getting the following error
|
@volodymyr-chekyrta I've figured out the issue. You need to add verified in the Mode enum.
|
Fixed. Please try now |
@volodymyr-chekyrta It's working now. Is it possible to make the parsing a bit lenient so that if an unknown value comes up, it will get ignored? Now I'm getting 404 for the API Response:
|
I'm a bit confused. A 404 response with a body. |
@saeedbashir, this endpoint is not related to this PR. Can we address this issue in a separate PR? |
It seems the issue is with the Examples:
Can you please handle the case and add a full-screen error on the course dashboard screen? and don't hit any API like blocks, etc if the user doesn't have access. Here is the sample response for courseware_access where the user does not have access.
|
Could you please confirm which endpoint you are referring to? |
The enrollments API The parsing of the enrollment API is also not working for a course It's very hard to put all the data values in the parsing list until we get all possible values for each key from the server, which seems impossible for now, and even a new value can be added in the future. We need to come up with a way to ignore the unknown values. |
I got it, thank you. I agree that this is very important behavior to handle access to courses; what if we create a ticket and handle this case in a separate PR, since this PR focuses on API switching but not on adding new functionality? |
Yes, we can handle this under a separate ticket. But I guess we need to handle the parsing for unknown values because the app behavior breaks on receiving an unknown value. |
Yes, I completely agree with that. |
It's failing for
|
Got it, thank you! |
@saeedbashir I've added a fallback to the enum; please try now, it should works |
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.
I've tested the highlighted APIs on the edX prod environment https://courses.edx.org
and they are working fine 👍
|
||
enum Org: String, Codable { | ||
case organization = "Organization" | ||
case univerity = "Univerity" |
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.
[curious] is this a typo that got missed? or is this a known edge case being handled this way?
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.
Not sure, need to check 🤔
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.
Fun fact, this enum is not used.
I don't know how it ended up there, but I've removed it from the code.
@volodymyr-chekyrta if all is well, then can you please merge this PR? |
@volodymyr-chekyrta 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
Migration API endpoints from the RG API plugin to the edx-platform upstream.