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

Learner metrics endpoint - Prerelease for evaluation #239

Merged
merged 2 commits into from
Aug 12, 2020

Conversation

johnbaldwin
Copy link
Contributor

@johnbaldwin johnbaldwin commented Aug 1, 2020

This PR is for the initial evaluation version of the learner metrics endpoint for Matej's implementation on the front end and evaluation in production.

The endpoint is /figures/api/learner-metrics/

  • There is a basic viewset just to exercise the code. The test requires test data to be filled out and tested in the response

  • UserFilterSet needs to be updated or an alternate filter set needs to be used in order to provide more filtering, in particular

  • Show only users who have enrollments

  • Show only users who do not have enrollments

  • Show only users who have completed

  • Show only users who have not completed

  • List serializers need to be added to prefetch data to improve API performance

  • test_learner_metrics_viewset needs to be completed

  • Updated the CourseEnrollment mock to provide the is_enrolled method

@johnbaldwin
Copy link
Contributor Author

I need to fix the unit test. It is failing because it is getting all users instead of the expected users. Should still work in devsite. I'll work on that tomorrow

results = response.data['results']
# Check user ids
result_ids = [obj['id'] for obj in results]
assert set(result_ids) == set([obj.user_id for obj in enrollments]+[caller.id])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is what is failing because the result_ids is also including users created in the base test class from which this class inherits

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is fixed

}
]
}
```
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@grozdanowski I pasted a sample from the mock results in devsite

@johnbaldwin johnbaldwin force-pushed the john/learner-metrics branch 2 times, most recently from fddaec3 to 5b69d15 Compare August 12, 2020 15:38
@codecov-commenter
Copy link

codecov-commenter commented Aug 12, 2020

Codecov Report

Merging #239 into master will increase coverage by 0.15%.
The diff coverage is 98.03%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #239      +/-   ##
==========================================
+ Coverage   91.62%   91.78%   +0.15%     
==========================================
  Files          41       41              
  Lines        2078     2129      +51     
==========================================
+ Hits         1904     1954      +50     
- Misses        174      175       +1     
Impacted Files Coverage Δ
figures/serializers.py 94.91% <97.22%> (+0.32%) ⬆️
figures/urls.py 100.00% <100.00%> (ø)
figures/views.py 92.68% <100.00%> (+0.28%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 42dddf6...1f7f714. Read the comment docs.

This is the initial commit so Matej and work on the front end

The endpoint is `/figures/api/learner-metrics/`

* There is a basic viewset just to exercise the code. The test requires
test data to be filled out and tested in the response

* UserFilterSet needs to be updated or an alternate filter set needs to
be used in order to provide more filtering, in particular
 * Show only users who have enrollments
 * Show only users who do not have enrollments
 * Show only users who have completed
 * Show only users who have not completed

* List serializers need to be added to prefetch data to improve API
performance
* test_learner_metrics_viewset needs to be completed
* Updated the CourseEnrollment mock to provide the `is_enrolled` method
@johnbaldwin johnbaldwin force-pushed the john/learner-metrics branch from 5b69d15 to 926f45d Compare August 12, 2020 15:49
@johnbaldwin johnbaldwin marked this pull request as ready for review August 12, 2020 15:59
@johnbaldwin johnbaldwin changed the title Learner metrics endpoint - Initial commit Learner metrics endpoint - Prerelease for evaluation Aug 12, 2020
Copy link
Contributor

@melvinsoft melvinsoft left a comment

Choose a reason for hiding this comment

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

@johnbaldwin I'm going a light review, I didn't spot checked any problem. Let's get this merged so we can test it. Thanks!

@johnbaldwin johnbaldwin merged commit a882e9b into master Aug 12, 2020
@johnbaldwin johnbaldwin deleted the john/learner-metrics branch September 12, 2020 10:49
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.

3 participants