(BB-1389) Update enrollment serializer and Add problem submission history endpoint by xirdneh · Pull Request #20948 · openedx/openedx-platform

xirdneh · 2019-07-03T22:28:53Z

Introduction

Another description as I don't have permission to edit the above.

This PR adds an endpoint displaying the submission history for each course a specific user is enrolled.

Sandbox URL: https://pr20948.sandbox.opencraft.hosting/ (provisioning)

Testing instructions

These can use the sandbox above or a devstack.

Ensure the logged in user has submitted a few problems.
Browse http://localhost:18000/api/enrollment/v1/submission_history or http://pr20948.sandbox.opencraft.hosting/api/enrollment/v1/submission_history
Make sure a JSON object showing the user submissions is displayed

Screenshots

Author notes and concerns

The update in the enrollment serializer makes extra mongodb calls.

openedx-webhooks · 2019-07-03T22:29:02Z

Thanks for the pull request, @xirdneh! I've created OSPR-3715 to keep track of it in JIRA, where we prioritize reviews. 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:

supporting documentation
Open edX discussion forum threads
timeline information ("this must be merged by XX date", and why that is)
partner information ("this is a course on edx.org")
any other information that can help Product understand the context for the PR

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.

kaizoku

I did some manual testing and things work alright, but I have a couple small comments and a question I'm not sure about.

kaizoku · 2019-07-15T09:35:47Z

        EnrollmentCourseDetailView.as_view(), name='courseenrollmentdetails'),
    url(r'^unenroll/$', UnenrollmentView.as_view(), name='unenrollment'),
    url(r'^roles/$', EnrollmentUserRolesView.as_view(), name='roles'),
+    url(r'^submission_history$', SubmissionHistoryView.as_view()),


This view should likely also have a name= parameter

@kaizoku What do you mean by a name= parameter?
Right now it accepts a user= parameter which is the username. If we add a name= parameter should it be user's name?

@kaizoku I've updated the view's documentation, hopefully that'll resolve any issues.

I mean in the url() definition here, like:

url(r'^submission_history$', SubmissionHistoryView.as_view(), name='submission_history'),

@kaizoku Ah, yes, let me add that :)

@kaizoku Added a view name.

Thanks @xirdneh

kaizoku · 2019-07-15T09:38:16Z

    """
    course_details = CourseSerializer(source="course_overview")
    user = serializers.SerializerMethodField('get_username')
+    finished = serializers.SerializerMethodField()


We should probably also specify the method name as is done for the user field.

@kaizoku we can't do this because there's a quality error raised:

AssertionError: It is redundant to specify `get_finished` on SerializerMethodField 'finished' in serializer 'CourseEnrollmentSerializer', because it is the same as the default method name. Remove the `method_name` argument.

Ah gotcha, my mistake.

kaizoku · 2019-07-15T09:40:35Z

+                    if section.get(u'prominent'):
+                        summary.append(section)
+            except PermissionDenied:
+                pass


This won't do anything if it fails permission checks, should we instead return an empty response?
This would wind up looking like a 0% grade instead, but perhaps that's the correct behavior, I'm not sure.

The values are initialized at the beginning of the method. If there's a permission issue the response would be:

{ 'current_grade': 0, 'certificate_eligible': False, 'summary': [], }

Right, I'm wondering if it would be better to imply an empty grade like that, or to return an empty list entirely.

@kaizoku I think to avoid issues on the frontend it's better to return an empty grade like this. That way the front-end code doesn't have to change if there's a permission error.
Now, I think probably a better solution would be to return a 403. You think that's acceptable?

I'm not sure, perhaps the edX reviewer will have some ideas on this?

@ormsbee Can you comment on this change? Is this approach feasible?

natabene · 2019-07-15T20:00:58Z

@xirdneh Thank you for your contribution. Is this ready for our review?

xirdneh · 2019-07-15T20:22:58Z

@natabene I believe this is ready for review, yes.

natabene · 2019-07-17T14:20:21Z

@ormsbee This is ready for your review when you have a chance.

viadanna · 2019-08-21T19:44:00Z

@edx/platform-core @ormsbee Friendly ping :)

giovannicimolin · 2019-12-11T20:08:31Z

jenkins run all

natabene · 2020-02-24T16:29:16Z

@xirdneh Can you provide more information about what this does?

viadanna · 2020-03-02T13:16:43Z

@natabene I'm taking over this PR. This essentially implements a new API endpoint to retrieve students problem submission history for a specific course. It includes necessary improvements to serialisers for the data being returned.

edx-status-bot · 2021-05-25T16:21:53Z

Your PR has finished running tests. The following contexts failed:

jenkins/a11y

arch-bom-gocd-alerts · 2021-06-11T18:04:06Z

📣 💥 Heads-up: You must either rebase onto master or merge master into your branch to avoid breaking the build.

We recently removed diff-quality and introduced lint-amnesty. This means that the automated quality check that has run on your branch doesn't work the same way it will on master. If you have introduced any quality failures, they might pass on the PR but then break the build on master.

This branch has been detected to not have commit 2e33565 as an ancestor. Here's how to see for yourself:

git merge-base --is-ancestor 2e335653 josuebc/upstream/BB-1389 && echo "You're all set" || echo "Please rebase onto master or merge master to your branch"

If you have any questions, please reach out to the Architecture team (either #edx-shared-architecture on Open edX Slack or #architecture on edX internal).

gabor-boros · 2021-06-17T17:16:03Z

@viadanna Could you please take a look at the a11y failures and the request to rebase?

viadanna · 2021-06-23T15:04:19Z

@viadanna Could you please take a look at the a11y failures and the request to rebase?

I'm looking into this, the PR is so old that it started failing without any changes. I'll schedule a couple hours to check this.

gabor-boros · 2021-07-20T10:48:06Z

Thank you for checking it @viadanna!

Agrendalath · 2021-10-02T10:53:29Z

+                pass
+        return [
+            {u'current_grade': current_grade,
+             u'certificate_eligible': course_grade.passed if course_grade else False,


This is not consistent with the certificates API, so it can return different values than generated grade reports.

Thanks for catching this @Agrendalath possibly one of the changes that have been impacting this PR since it has been created. I'll schedule some time to look into this, resolve conflicts and fix tests.

mtyaka · 2022-02-15T09:59:04Z

Closing as the code become stale and we no longer need these changes at OpenCraft.

openedx-webhooks · 2022-02-15T09:59:10Z

@xirdneh Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future.

… endpoint (squashed and cherry picked from openedx#20948) NOTE 1: during a next release preparation, don't cherry-pick the original commit, as they are heavily outdated. Cherry-pick this one, as it has linting and compatibility fixes. NOTE 2: `max_mongo_calls` in `EnrollmentTestMixin` was bumped from 8 to 12.

(BB-1389) Update enrollment serializer and Add problem submission history endpoint Squashed all commits from https://github.com/edx/edx-platform/pull/20948 (cherry picked from commit c102e41c6d92e5fb7db66de4d3624f03da213631) This cherry-pick was modified and updates works with mango.

(BB-1389) Update enrollment serializer and Add problem submission history endpoint Squashed all commits from https://github.com/edx/edx-platform/pull/20948 (cherry picked from commit c102e41c6d92e5fb7db66de4d3624f03da213631) This cherry-pick was modified and updated to work with mango.

openedx-webhooks added needs triage open-source-contribution PR author is not from Axim or 2U labels Jul 3, 2019

xirdneh force-pushed the josuebc/upstream/BB-1389 branch from 1e29e62 to 0a1f0bf Compare July 11, 2019 15:35

xirdneh requested a review from a team July 12, 2019 14:48

xirdneh force-pushed the josuebc/upstream/BB-1389 branch from 3f8f529 to 6d3afeb Compare July 15, 2019 02:01

xirdneh changed the title ~~[WIP] (BB-1389) Update enrollment serializer and Add problem submission history endpoint~~ (BB-1389) Update enrollment serializer and Add problem submission history endpoint Jul 15, 2019

kaizoku suggested changes Jul 15, 2019

View reviewed changes

xirdneh force-pushed the josuebc/upstream/BB-1389 branch from 4032ebb to abbe321 Compare July 15, 2019 14:13

openedx-webhooks added waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. and removed needs triage labels Jul 15, 2019

natabene requested a review from ormsbee July 17, 2019 14:20

openedx-webhooks added awaiting prioritization and removed waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Jul 17, 2019

openedx-webhooks added waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. and removed awaiting prioritization labels Feb 24, 2020

openedx-webhooks added engineering review awaiting prioritization and removed waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. engineering review labels Mar 3, 2020

viadanna force-pushed the josuebc/upstream/BB-1389 branch from 3a36ee0 to f12c39f Compare March 13, 2020 12:40

viadanna force-pushed the josuebc/upstream/BB-1389 branch 2 times, most recently from 09a2ec1 to d5c5e67 Compare March 24, 2020 19:54

Agrendalath reviewed Oct 2, 2021

View reviewed changes

mtyaka closed this Feb 15, 2022

openedx-webhooks removed the awaiting prioritization label Feb 15, 2022

openedx-webhooks added the rejected label Feb 15, 2022

0x29a deleted the josuebc/upstream/BB-1389 branch September 22, 2022 10:01

0x29a mentioned this pull request Sep 22, 2022

[BB-6692] Prepare opencraft-release/nutmeg.2 branch for edx-platform open-craft/openedx-platform#490

Merged

johanseto mentioned this pull request Oct 5, 2022

feat: submision history endpoint from koa nelp eduNEXT/edunext-platform#679

Open

Agrendalath mentioned this pull request Jan 6, 2023

feat: Added submission history API and extended enrollment API to support grading and finished flags. #31491

Merged

1 task

Uh oh!

Conversation

xirdneh commented Jul 3, 2019 • edited by bradenmacdonald Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Introduction

Testing instructions

Screenshots

Author notes and concerns

Uh oh!

openedx-webhooks commented Jul 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kaizoku left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xirdneh Jul 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xirdneh Jul 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

natabene commented Jul 15, 2019

Uh oh!

xirdneh commented Jul 15, 2019

Uh oh!

natabene commented Jul 17, 2019

Uh oh!

viadanna commented Aug 21, 2019

Uh oh!

giovannicimolin commented Dec 11, 2019

Uh oh!

natabene commented Feb 24, 2020

Uh oh!

viadanna commented Mar 2, 2020

Uh oh!

edx-status-bot commented May 25, 2021

Uh oh!

arch-bom-gocd-alerts commented Jun 11, 2021

Uh oh!

gabor-boros commented Jun 17, 2021

Uh oh!

viadanna commented Jun 23, 2021

Uh oh!

gabor-boros commented Jul 20, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtyaka commented Feb 15, 2022

xirdneh commented Jul 3, 2019 •

edited by bradenmacdonald

Loading

openedx-webhooks commented Jul 3, 2019 •

edited

Loading

xirdneh Jul 15, 2019 •

edited

Loading

xirdneh Jul 15, 2019 •

edited

Loading