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

✨(api) implement video new downloads endpoint #44

Merged
merged 7 commits into from
Jul 26, 2023
Merged

✨(api) implement video new downloads endpoint #44

merged 7 commits into from
Jul 26, 2023

Conversation

lebaudantoine
Copy link
Contributor

@lebaudantoine lebaudantoine commented Jul 23, 2023 •
edited
Loading

Purpose

Implement a downloads endpoint for the video plugin, inspired by @lebrunthibault's work in PR #28.

Proposal

Closes #24

Downloads events are described by the xAPI verb Downloaded from tincanapi. This xAPI verb is available in ralph-malp==3.8.0.

This new endpoint follows the same code structure as the views one, implemented by @cyrillay.

  • Upgrade Ralph to 3.8.
  • Create downloads Models.
  • Create downloads Indicator.
  • Create downloads Endpoint.
  • Add tests.

@lebaudantoine lebaudantoine self-assigned this Jul 23, 2023
@lebaudantoine
Copy link
Contributor Author

lebaudantoine commented Jul 23, 2023 •
edited
Loading

Note: I suggest unifying the naming of actor.uuid to improve clarity. Currently, we have three different names in the codebase:

  1. actor.account.name
  2. unique_id
  3. uuid

The most suitable option among them is uuid, which is used to label the newly added column.

@lebaudantoine
Copy link
Contributor Author

lebaudantoine commented Jul 23, 2023 •
edited
Loading

@jmaupetit, let's discuss refactoring. Although we maintain the duplication for the indicators, I strongly recommend abstracting the views and downloads models into more general pydantic models to reduce duplication. These models are quite naive.

@jmaupetit
Copy link
Contributor

The most suitable option among them is uuid, which is used to label the newly added column.

I would suggest uid (unique identifier) instead to avoid confusion as its not a real UUID.

@jmaupetit
Copy link
Contributor

@jmaupetit, let's discuss refactoring. Although we maintain the duplication for the indicators, I strongly recommend abstracting the views and downloads models into more general pydantic models to reduce duplication. These models are quite naive.

I do agree!

@lebaudantoine lebaudantoine force-pushed the feature/endpoint-downloads branch 6 times, most recently from a6ce217 to 53000c8 Compare July 24, 2023 18:42
@lebaudantoine lebaudantoine marked this pull request as ready for review July 24, 2023 18:50
@lebaudantoine lebaudantoine changed the title Feature/endpoint downloads ✨(api) implement video new downloads endpoint Jul 24, 2023
jmaupetit
jmaupetit reviewed Jul 25, 2023
View reviewed changes
Copy link
Contributor

@jmaupetit jmaupetit left a comment

Choose a reason for hiding this comment

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

As you proposed a new indicator, we clearly see that we need to think a bit more our semantic and factoring. Thank you for highlighting this. 💪 😎

src/api/plugins/video/tests/test_api.py Show resolved Hide resolved
src/api/plugins/video/tests/test_api.py Outdated Show resolved Hide resolved
src/api/plugins/video/warren_video/indicators.py Show resolved Hide resolved
src/api/plugins/video/warren_video/models.py Outdated Show resolved Hide resolved
src/api/plugins/video/warren_video/models.py Outdated Show resolved Hide resolved
src/api/plugins/video/warren_video/api.py Show resolved Hide resolved
@lebaudantoine lebaudantoine force-pushed the feature/endpoint-downloads branch 3 times, most recently from 53000c8 to 8df9b55 Compare July 26, 2023 14:02
@lebaudantoine
⬆️(api) bump ralph-malp to v3.8
3ebac8f 
Upgrade ralph-malp to import a new xAPI verb.
Downloaded, from tincanapi is required to add the
new endpoint downloads.
@lebaudantoine
✨(api) implement video new downloads endpoint
6495acd 
Enhance engagement metrics by implementing a new endpoint to track download
events on video resources. This endpoint queries an LRS for statements with
the 'Downloaded' xAPI verb (http://id.tincanapi.com/verb/downloaded).

The endpoint provides both a total count of events and a daily count for
studying download patterns.
@lebaudantoine
♻️(api) avoid confusion while designating actor's identifier
ebb272f 
To improve clarity and avoid confusion, we propose unifying the naming of
actor.uuid to use uid throughout the codebase. This change will provide a
consistent and clear representation, emphasizing that the identifier is unique,
though not necessarily universal.
@lebaudantoine
♻️(api) merge video views and downloads pydantic models
ddcca66 
Current specific classes were not adding significant value beyond the
proposed base DailyCount and DailyCounts classes. They served as
aliases with different names, which led to redundant code.
@lebaudantoine
🩹(api) fix mis-renamed endpoint parameter in tests
a61c44b 
video_id was renamed to video_uuid whithout thorough attention.
Align all existing API tests.
@lebaudantoine
♻️(api) update timestamps fixtures to Ralph's format
4da99ed 
After sutdying some raw statements from ralph, I noticed
that statements' timestamps were stored as date and time
string with an offset from UTC, and not as UTC.
@lebaudantoine
✅(api) add tests on video new downloads endpoint
97e6e57 
Current tests are following the exact same structure as
the `views` ones. Please note that these tests are a basis
and should be extended with more advanced one as soon as the
API complexity and usage increase.
jmaupetit
jmaupetit approved these changes Jul 26, 2023
View reviewed changes
Copy link
Contributor

@jmaupetit jmaupetit left a comment

Choose a reason for hiding this comment

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

src/api/core/warren/base_indicator.py Outdated Show resolved Hide resolved
src/api/core/warren/base_indicator.py Outdated Show resolved Hide resolved
src/api/core/pyproject.toml Show resolved Hide resolved
@lebaudantoine lebaudantoine merged commit 31070c5 into main Jul 26, 2023
@lebaudantoine lebaudantoine deleted the feature/endpoint-downloads branch July 26, 2023 15:21
@Leobouloc Leobouloc mentioned this pull request Jul 26, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jmaupetit jmaupetit jmaupetit approved these changes

Assignees

@lebaudantoine lebaudantoine

Labels
feature
Projects
None yet
Milestone
No milestone
2 participants
@lebaudantoine @jmaupetit