-
Notifications
You must be signed in to change notification settings - Fork 4.3k
feat: History log logic for Components and Containers [FC-0123] #38178
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
base: master
Are you sure you want to change the base?
Changes from all commits
9e5a1c5
80c9155
e20f0a3
0fdbd38
1688454
c6df70f
df20d13
c14bb36
1596aae
b0df5d3
e89065d
bfc0013
d0b6263
7cd6457
cc87727
e9e9061
2518487
93be604
2345adf
8f7a67e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,18 +4,28 @@ | |
| from __future__ import annotations | ||
|
|
||
| from dataclasses import dataclass | ||
| from datetime import datetime | ||
|
|
||
| from django.contrib.auth import get_user_model | ||
| from django.utils.translation import gettext as _ # noqa: F401 | ||
| from opaque_keys.edx.locator import LibraryUsageLocatorV2 | ||
| from opaque_keys.edx.locator import LibraryLocatorV2, LibraryUsageLocatorV2 | ||
| from openedx_content.models_api import Component, PublishLogRecord | ||
|
|
||
| from openedx.core.djangoapps.user_api.accounts.image_helpers import get_profile_image_urls_for_user | ||
|
|
||
| from .libraries import PublishableItem, library_component_usage_key | ||
|
|
||
| # The public API is only the following symbols: | ||
| __all__ = [ | ||
| "LibraryXBlockMetadata", | ||
| "LibraryXBlockStaticFile", | ||
| "LibraryHistoryEntry", | ||
| "LibraryHistoryContributor", | ||
| "LibraryPublishHistoryGroup", | ||
| ] | ||
|
|
||
| User = get_user_model() | ||
|
|
||
|
|
||
| @dataclass(frozen=True, kw_only=True) | ||
| class LibraryXBlockMetadata(PublishableItem): | ||
|
|
@@ -48,6 +58,7 @@ def from_component(cls, library_key, component, associated_collections=None): | |
| usage_key=usage_key, | ||
| display_name=draft.title, | ||
| created=component.created, | ||
| created_by=component.created_by.username if component.created_by else None, | ||
| modified=draft.created, | ||
| draft_version_num=draft.version_num, | ||
| published_version_num=published.version_num if published else None, | ||
|
|
@@ -63,6 +74,77 @@ def from_component(cls, library_key, component, associated_collections=None): | |
| ) | ||
|
|
||
|
|
||
| @dataclass(frozen=True) | ||
| class LibraryHistoryEntry: | ||
| """ | ||
| One entry in the history of a library component. | ||
| """ | ||
| changed_by: LibraryHistoryContributor | None | ||
| changed_at: datetime | ||
| title: str # title at time of change | ||
| item_type: str | ||
| action: str # "edited" | "renamed" | ||
|
|
||
|
|
||
| @dataclass(frozen=True) | ||
| class LibraryHistoryContributor: | ||
| """ | ||
| A contributor in a publish history group, with profile image URLs. | ||
| """ | ||
| username: str | ||
| profile_image_urls: dict # {"full": str, "large": str, "medium": str, "small": str} | ||
|
|
||
| @classmethod | ||
| def from_user(cls, user, request=None) -> LibraryHistoryContributor: | ||
| return cls( | ||
| username=user.username, | ||
| profile_image_urls=get_profile_image_urls_for_user(user, request), | ||
| ) | ||
|
|
||
|
|
||
| @dataclass(frozen=True) | ||
| class DirectPublishedEntity: | ||
| """ | ||
| Represents one entity the user directly requested to publish (direct=True). | ||
| Each entry carries its own title and entity_type so the frontend can display | ||
| the correct label for each directly published item. | ||
|
|
||
| Pre-Verawood groups have exactly one entry (approximated from available data). | ||
| Post-Verawood groups have one entry per direct=True record in the PublishLog. | ||
| """ | ||
| entity_key: str # str(usage_key) for components, str(container_key) for containers | ||
| title: str # title of the entity at time of publish | ||
| entity_type: str # e.g. "html", "problem" for components; "unit", "section" for containers | ||
|
|
||
|
|
||
| @dataclass(frozen=True) | ||
| class LibraryPublishHistoryGroup: | ||
| """ | ||
| Summary of a publish event for a library item. | ||
|
|
||
| Each instance represents one or more PublishLogRecords, and includes the | ||
| set of contributors who authored draft changes between the previous publish | ||
| and this one. | ||
|
|
||
| Pre-Verawood (direct=None): one group per entity × publish event. | ||
| Post-Verawood (direct!=None): one group per unique PublishLog. | ||
| """ | ||
| publish_log_uuid: str | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar above, why not have this be a UUID? |
||
| published_by: object # AUTH_USER_MODEL instance or None | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @kdmccormick: What's the right way to do this if we want to be respectful of custom user models? I vaguely recall you went down this rabbit hole once.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, I think openedx-platform has too many assumptions about our user model for custom user models to work anyway, so maybe we can just make this a User? |
||
| published_at: datetime | ||
| contributors: list[LibraryHistoryContributor] # distinct authors of versions in this group | ||
| contributors_count: int | ||
| # Replaces entity_key, title, block_type. Each element is one entity the | ||
| # user directly requested to publish. Pre-Verawood: single approximated entry. | ||
| # Post-Verawood: one entry per direct=True record in the PublishLog. | ||
| direct_published_entities: list[DirectPublishedEntity] | ||
| # Key to pass as scope_entity_key when fetching entries for this group. | ||
| # Pre-Verawood: the specific entity key for this group (container or usage key). | ||
| # Post-Verawood container groups: None — frontend must use currentContainerKey. | ||
| # Component history (all eras): str(usage_key). | ||
| scope_entity_key: str | None | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can this be a union of the acceptable key types? |
||
|
|
||
|
|
||
| @dataclass(frozen=True) | ||
| class LibraryXBlockStaticFile: | ||
| """ | ||
|
|
@@ -76,3 +158,74 @@ class LibraryXBlockStaticFile: | |
| url: str | ||
| # Size in bytes | ||
| size: int | ||
|
|
||
|
|
||
| def resolve_contributors(users, request=None) -> list[LibraryHistoryContributor | None]: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. At a high level, it seems like the whole function of this is to make sure that whatever users come in do a [
LibraryHistoryContributor.from_user(user, request) if user else None
for user in users
]Am I interpreting that correctly? If so, can we just make sure the original qset uses the right |
||
| """ | ||
| Convert an iterable of User objects (possibly containing None) to a list of | ||
| LibraryHistoryContributor. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please put more information in the docstring about how this function is expected to behave, such as:
|
||
| """ | ||
| users_list = list(users) | ||
| user_pks = list({user.pk for user in users_list if user is not None}) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
if not user_pks:
return [] |
||
| prefetched = { | ||
| user.pk: user | ||
| for user in User.objects.filter(pk__in=user_pks).select_related('profile') | ||
| } if user_pks else {} | ||
| return [ | ||
| LibraryHistoryContributor.from_user(prefetched.get(user.pk, user), request) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| if user else None | ||
| for user in users_list | ||
| ] | ||
|
|
||
|
|
||
| def resolve_change_action(old_version, new_version) -> str: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please annotate what types |
||
| """ | ||
| Derive a human-readable action label from a draft history record's versions. | ||
|
|
||
| Returns "renamed" when both versions exist and the title changed between | ||
| them; otherwise returns "edited" as the default action. | ||
|
Comment on lines
+185
to
+186
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are creation and deletion out of scope? |
||
| """ | ||
| if old_version and new_version and old_version.title != new_version.title: | ||
| return "renamed" | ||
| return "edited" | ||
|
|
||
|
|
||
| def direct_published_entity_from_record( | ||
| record: PublishLogRecord, | ||
| lib_key: LibraryLocatorV2, | ||
| ) -> DirectPublishedEntity: | ||
| """ | ||
| Build a DirectPublishedEntity from a PublishLogRecord. | ||
|
|
||
| lib_key is used only to construct locator strings — entity_key is always | ||
| derived from record.entity itself, never from an external container key. | ||
|
|
||
| Callers must ensure the record is fetched with: | ||
| select_related( | ||
| 'entity__component__component_type', | ||
| 'entity__container__container_type', | ||
| 'new_version', | ||
| ) | ||
| """ | ||
| # Import here to avoid circular imports (container_metadata imports block_metadata). | ||
| from .container_metadata import library_container_locator # noqa: PLC0415 | ||
|
|
||
| title = record.new_version.title if record.new_version else "" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does that mean that deletion is going to show up in the log as: "" was edited Because the |
||
| try: | ||
| component = record.entity.component | ||
| return DirectPublishedEntity( | ||
| entity_key=str(LibraryUsageLocatorV2( # type: ignore[abstract] | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is |
||
| lib_key=lib_key, | ||
| block_type=component.component_type.name, | ||
| usage_id=component.component_code, | ||
| )), | ||
| title=title, | ||
| entity_type=component.component_type.name, | ||
| ) | ||
| except Component.DoesNotExist: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please do checks with |
||
| container = record.entity.container | ||
| return DirectPublishedEntity( | ||
| entity_key=str(library_container_locator(lib_key, container)), | ||
| title=title, | ||
| entity_type=container.container_type.type_code, | ||
| ) | ||
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.
Why make this a
strinstead of aLibraryUsageLocatorV2?