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.

Sign up for GitHub

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

Jump to bottom

Expose annotations stats grouped by assignments for LMS #8709

Merged
merged 2 commits into from
May 20, 2024
Merged

Expose annotations stats grouped by assignments for LMS #8709

merged 2 commits into from
May 20, 2024

Conversation

marcospri
Copy link
Member

@marcospri marcospri commented May 16, 2024 •
edited

@marcospri marcospri mentioned this pull request May 16, 2024
Merged
marcospri
marcospri commented May 16, 2024
View reviewed changes
h/services/bulk_api/lms_stats.py Outdated

annos_query = (
self._annotation_type_select()
def _annotation_query(self, groups: list[str], assignment_id: str | None = None):
Copy link
Member Author

Choose a reason for hiding this comment

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

A small, refactor, make the helper method bigger and parameterize parts of the query.

h/services/bulk_api/lms_stats.py Outdated

return query

def assignment_stats(
Copy link
Member Author

Choose a reason for hiding this comment

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

Existing method, use the new helper method.

h/services/bulk_api/lms_stats.py Outdated
@@ -106,6 +121,37 @@ def assignment_stats(
for row in results
]

def course_stats(self, groups: list[str]) -> list[CourseStats]:
Copy link
Member Author

Choose a reason for hiding this comment

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

New method, groups the existing query by something else (assignment instead of user)

h/views/api/bulk/stats.py Outdated
data = CourseStatsSchema().validate(request.json)
query_filter = data["filter"]

stats = request.find_service(BulkLMSStatsService).course_stats(
Copy link
Member Author

Choose a reason for hiding this comment

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

Expose the new service method in the API

@marcospri marcospri requested a review from acelaya May 16, 2024 12:02
acelaya
acelaya reviewed May 17, 2024
View reviewed changes
Copy link
Contributor

@acelaya acelaya left a comment

Choose a reason for hiding this comment

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

Makes sense in general, but I left a few comments.

h/services/bulk_api/lms_stats.py Outdated
Comment on lines 21 to 25
class CourseStats:
assignment_id: str
annotations: int
replies: int
last_activity: datetime
Copy link
Contributor

Choose a reason for hiding this comment

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

According to the props, this class seems to represent the stats for an individual assignment. Should it be called AssignmentStats then?

I suppose you named it CourseStats, because the "api.bulk.stats.course" endpoint returns a list of these, but I think it might be confusing long term.

Copy link
Member Author

Choose a reason for hiding this comment

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

I changed naming around to make reference to what they return (users or assignments) instead of the context they are called in (course / assignment).

If we go the route we have discussed we might return different things on different context so this will more future proof and easier to understand.

h/views/api/bulk/stats.py
@api_config(
versions=["v1", "v2"],
route_name="api.bulk.stats.course",
request_method="POST",
Copy link
Contributor

Choose a reason for hiding this comment

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

Considering this endpoint has no side effects, should it be using GET and receive the filtering options via query params?

I know that's sometimes a bit limiting and a JSON body is more flexible, so we don't have to be super REST compliant if being pragmatic makes more sense here.

If only the QUERY method was already a thing 😅

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I reckon these make sense as POST for now event if it's very un-RESTFUL.

At least this is an internal API, we should be more careful in public APIs.

h/services/bulk_api/lms_stats.py Outdated
Comment on lines 74 to 76
if assignment_id:
query = query.where(
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of having this condition here and an optional assignment_id param, you could leve only the common parts here, and move the assignment_id condition to assignment_stats().

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

marcospri
marcospri commented May 17, 2024
View reviewed changes
h/services/bulk_api/lms_stats.py Outdated
@@ -17,65 +17,83 @@ class AssignmentStats:
last_activity: datetime


@dataclass
class CourseStats:
Copy link
Member Author

Choose a reason for hiding this comment

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

The diff is not showing this to me right now but instead of `CourseStats" this is now "CountsByAssignment"

marcospri
marcospri commented May 17, 2024
View reviewed changes
h/routes.py
@@ -134,8 +134,11 @@ def includeme(config): # pylint: disable=too-many-statements
config.add_route("api.bulk.group", "/api/bulk/group", request_method="POST")

config.add_route(
"api.bulk.stats.assignment",
"/api/bulk/stats/assignment",
"api.bulk.stats.users", "/api/bulk/stats/users", request_method="POST"
Copy link
Member Author

Choose a reason for hiding this comment

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

Naming reflects what these return.

marcospri
marcospri commented May 17, 2024
View reviewed changes
h/services/bulk_api/lms_stats.py
annotations: int
replies: int
last_activity: datetime


@dataclass
class CountsByUser(_AnnotationCounts):
Copy link
Member Author

Choose a reason for hiding this comment

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

More explicit naming. I reckon this is much more clear.

@marcospri marcospri requested a review from acelaya May 20, 2024 10:59
acelaya
acelaya approved these changes May 20, 2024
View reviewed changes
Copy link
Contributor

@acelaya acelaya left a comment

Choose a reason for hiding this comment

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

👍🏼

@marcospri marcospri merged commit 3545284 into main May 20, 2024
15 checks passed
@marcospri marcospri deleted the annos-by-assignment branch May 20, 2024 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@acelaya acelaya acelaya approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@marcospri @acelaya