diff --git a/RELEASE.rst b/RELEASE.rst index e05a6b42e7..da572a6b2b 100644 --- a/RELEASE.rst +++ b/RELEASE.rst @@ -1,6 +1,13 @@ Release Notes ============= +Version 0.80.15 +--------------- + +- credential metadata task (#3950) +- Skip unreferenced static files when ingesting edX course archives (#3942) +- Mark the Django session cookie Secure by default (#3968) + Version 0.80.14 --------------- diff --git a/env/backend.env b/env/backend.env index 82f45d76b3..10ef493e41 100644 --- a/env/backend.env +++ b/env/backend.env @@ -11,6 +11,7 @@ CORS_ALLOWED_ORIGINS='["http://open.odl.local:8062"]' CSRF_TRUSTED_ORIGINS='["http://open.odl.local:8062", "http://api.open.odl.local:8063"]' CSRF_COOKIE_DOMAIN=open.odl.local CSRF_COOKIE_SECURE=False +SESSION_COOKIE_SECURE=False MITOL_COOKIE_DOMAIN=open.odl.local MITOL_COOKIE_NAME=mitlearn diff --git a/frontends/api/src/generated/v0/api.ts b/frontends/api/src/generated/v0/api.ts index 57cdd85e51..bef5eb1592 100644 --- a/frontends/api/src/generated/v0/api.ts +++ b/frontends/api/src/generated/v0/api.ts @@ -3504,7 +3504,7 @@ export const CredentialMetadataApiAxiosParamCreator = function ( ) { return { /** - * Generate Open Badges credential metadata for a learning resource. Limited to MITx Online courses. + * Read or generate Open Badges credential metadata for a learning resource. Limited to MITx Online courses. * @summary Generate credential metadata * @param {CredentialMetadataRequestRequest} CredentialMetadataRequestRequest * @param {*} [options] Override http request option. @@ -3553,6 +3553,59 @@ export const CredentialMetadataApiAxiosParamCreator = function ( configuration, ) + return { + url: toPathString(localVarUrlObj), + options: localVarRequestOptions, + } + }, + /** + * Read or generate Open Badges credential metadata for a learning resource. Limited to MITx Online courses. + * @summary Get stored credential metadata + * @param {string} resource_readable_id The readable id of the learning resource to fetch stored metadata for + * @param {*} [options] Override http request option. + * @throws {RequiredError} + */ + credentialMetadataRetrieve: async ( + resource_readable_id: string, + options: RawAxiosRequestConfig = {}, + ): Promise => { + // verify required parameter 'resource_readable_id' is not null or undefined + assertParamExists( + "credentialMetadataRetrieve", + "resource_readable_id", + resource_readable_id, + ) + const localVarPath = `/api/v0/credential_metadata/` + // use dummy base URL string because the URL constructor only accepts absolute URLs. + const localVarUrlObj = new URL(localVarPath, DUMMY_BASE_URL) + let baseOptions + if (configuration) { + baseOptions = configuration.baseOptions + } + + const localVarRequestOptions = { + method: "GET", + ...baseOptions, + ...options, + } + const localVarHeaderParameter = {} as any + const localVarQueryParameter = {} as any + + if (resource_readable_id !== undefined) { + localVarQueryParameter["resource_readable_id"] = resource_readable_id + } + + localVarHeaderParameter["Accept"] = "application/json" + + setSearchParams(localVarUrlObj, localVarQueryParameter) + let headersFromBaseOptions = + baseOptions && baseOptions.headers ? baseOptions.headers : {} + localVarRequestOptions.headers = { + ...localVarHeaderParameter, + ...headersFromBaseOptions, + ...options.headers, + } + return { url: toPathString(localVarUrlObj), options: localVarRequestOptions, @@ -3571,7 +3624,7 @@ export const CredentialMetadataApiFp = function ( CredentialMetadataApiAxiosParamCreator(configuration) return { /** - * Generate Open Badges credential metadata for a learning resource. Limited to MITx Online courses. + * Read or generate Open Badges credential metadata for a learning resource. Limited to MITx Online courses. * @summary Generate credential metadata * @param {CredentialMetadataRequestRequest} CredentialMetadataRequestRequest * @param {*} [options] Override http request option. @@ -3604,6 +3657,40 @@ export const CredentialMetadataApiFp = function ( configuration, )(axios, localVarOperationServerBasePath || basePath) }, + /** + * Read or generate Open Badges credential metadata for a learning resource. Limited to MITx Online courses. + * @summary Get stored credential metadata + * @param {string} resource_readable_id The readable id of the learning resource to fetch stored metadata for + * @param {*} [options] Override http request option. + * @throws {RequiredError} + */ + async credentialMetadataRetrieve( + resource_readable_id: string, + options?: RawAxiosRequestConfig, + ): Promise< + ( + axios?: AxiosInstance, + basePath?: string, + ) => AxiosPromise + > { + const localVarAxiosArgs = + await localVarAxiosParamCreator.credentialMetadataRetrieve( + resource_readable_id, + options, + ) + const localVarOperationServerIndex = configuration?.serverIndex ?? 0 + const localVarOperationServerBasePath = + operationServerMap[ + "CredentialMetadataApi.credentialMetadataRetrieve" + ]?.[localVarOperationServerIndex]?.url + return (axios, basePath) => + createRequestFunction( + localVarAxiosArgs, + globalAxios, + BASE_PATH, + configuration, + )(axios, localVarOperationServerBasePath || basePath) + }, } } @@ -3618,7 +3705,7 @@ export const CredentialMetadataApiFactory = function ( const localVarFp = CredentialMetadataApiFp(configuration) return { /** - * Generate Open Badges credential metadata for a learning resource. Limited to MITx Online courses. + * Read or generate Open Badges credential metadata for a learning resource. Limited to MITx Online courses. * @summary Generate credential metadata * @param {CredentialMetadataApiCredentialMetadataCreateRequest} requestParameters Request parameters. * @param {*} [options] Override http request option. @@ -3635,6 +3722,24 @@ export const CredentialMetadataApiFactory = function ( ) .then((request) => request(axios, basePath)) }, + /** + * Read or generate Open Badges credential metadata for a learning resource. Limited to MITx Online courses. + * @summary Get stored credential metadata + * @param {CredentialMetadataApiCredentialMetadataRetrieveRequest} requestParameters Request parameters. + * @param {*} [options] Override http request option. + * @throws {RequiredError} + */ + credentialMetadataRetrieve( + requestParameters: CredentialMetadataApiCredentialMetadataRetrieveRequest, + options?: RawAxiosRequestConfig, + ): AxiosPromise { + return localVarFp + .credentialMetadataRetrieve( + requestParameters.resource_readable_id, + options, + ) + .then((request) => request(axios, basePath)) + }, } } @@ -3645,12 +3750,22 @@ export interface CredentialMetadataApiCredentialMetadataCreateRequest { readonly CredentialMetadataRequestRequest: CredentialMetadataRequestRequest } +/** + * Request parameters for credentialMetadataRetrieve operation in CredentialMetadataApi. + */ +export interface CredentialMetadataApiCredentialMetadataRetrieveRequest { + /** + * The readable id of the learning resource to fetch stored metadata for + */ + readonly resource_readable_id: string +} + /** * CredentialMetadataApi - object-oriented interface */ export class CredentialMetadataApi extends BaseAPI { /** - * Generate Open Badges credential metadata for a learning resource. Limited to MITx Online courses. + * Read or generate Open Badges credential metadata for a learning resource. Limited to MITx Online courses. * @summary Generate credential metadata * @param {CredentialMetadataApiCredentialMetadataCreateRequest} requestParameters Request parameters. * @param {*} [options] Override http request option. @@ -3667,6 +3782,25 @@ export class CredentialMetadataApi extends BaseAPI { ) .then((request) => request(this.axios, this.basePath)) } + + /** + * Read or generate Open Badges credential metadata for a learning resource. Limited to MITx Online courses. + * @summary Get stored credential metadata + * @param {CredentialMetadataApiCredentialMetadataRetrieveRequest} requestParameters Request parameters. + * @param {*} [options] Override http request option. + * @throws {RequiredError} + */ + public credentialMetadataRetrieve( + requestParameters: CredentialMetadataApiCredentialMetadataRetrieveRequest, + options?: RawAxiosRequestConfig, + ) { + return CredentialMetadataApiFp(this.configuration) + .credentialMetadataRetrieve( + requestParameters.resource_readable_id, + options, + ) + .then((request) => request(this.axios, this.basePath)) + } } /** diff --git a/learning_resources/admin.py b/learning_resources/admin.py index eab25890c8..6204a9c421 100644 --- a/learning_resources/admin.py +++ b/learning_resources/admin.py @@ -338,6 +338,16 @@ def has_delete_permission(self, request, obj=None): # noqa: ARG002 return False +class CredentialMetadataAdmin(admin.ModelAdmin): + """CredentialMetadata Admin""" + + model = models.CredentialMetadata + list_display = ("learning_resource", "description", "created_on", "updated_on") + search_fields = ("learning_resource__readable_id", "learning_resource__title") + readonly_fields = ("created_on", "updated_on") + raw_id_fields = ("learning_resource",) + + admin.site.register(models.LearningResourceTopic, LearningResourceTopicAdmin) admin.site.register(models.LearningResourceInstructor, LearningResourceInstructorAdmin) admin.site.register(models.LearningResource, LearningResourceAdmin) @@ -360,3 +370,4 @@ def has_delete_permission(self, request, obj=None): # noqa: ARG002 admin.site.register( models.CredentialMetadataGenerationLog, CredentialMetadataGenerationLogAdmin ) +admin.site.register(models.CredentialMetadata, CredentialMetadataAdmin) diff --git a/learning_resources/credentials.py b/learning_resources/credentials.py index b988ea8546..aac48c24de 100644 --- a/learning_resources/credentials.py +++ b/learning_resources/credentials.py @@ -15,6 +15,7 @@ from typing_extensions import TypedDict from learning_resources.constants import CredentialMetadataField +from learning_resources.credentials_store import save_credential_metadata from learning_resources.etl.constants import MARKETING_PAGE_FILE_TYPE from learning_resources.models import ( ContentFile, @@ -277,10 +278,6 @@ async def _retrieve_chunks( ) -> list[tuple[str, str]]: """ Retrieve the resource's most relevant content-file chunks. - - Retrieval is best-effort: metadata plus the marketing page is a viable - degraded context, so a Qdrant outage returns a thinner draft rather than an - error. """ try: chunks = await async_content_file_chunks_for_resource( @@ -290,8 +287,7 @@ async def _retrieve_chunks( ) except Exception: logger.exception( - "Content file retrieval failed for %s; generating from metadata" - " and marketing page alone", + "Content file retrieval failed for %s; skipping generation", resource.readable_id, ) return [] @@ -328,6 +324,38 @@ async def build_credential_context( ) +def _missing_context_sources( + context: CredentialContext, *, retrieved: bool +) -> list[str]: + """ + Return the sources the configurations ask for that the context lacks. + + Generating without them is worse than not generating: the output reads + like any other result, but a description written from the resource's own + metadata alone, or criteria with none of the course's content behind them, + is not something to issue a credential from. Returning empty-handed + leaves the resource with no stored metadata, so the daily sweep picks it + up again once its marketing page is scraped or its content indexed. + + Args: + context (CredentialContext): the assembled sources + retrieved (bool): whether content retrieval was attempted. A + configuration with a blank retrieval_query is asking for + generation from the marketing page alone, so empty chunks are not + a missing source -- nothing was asked for. + + Returns: + list of str: the missing sources, named for a log line and an error + message. Empty means the context is complete. + """ + missing = [] + if not context.marketing_page: + missing.append("marketing page") + if retrieved and not context.chunks: + missing.append("course content") + return missing + + def _get_llm(config: CredentialMetadataConfiguration) -> ChatLiteLLM: """ Get the ChatLiteLLM instance for a field's configuration. @@ -416,11 +444,28 @@ async def _generate_field( return FieldOutcome(response=response, error=error) +def _active_configs(fields: list[str] | None) -> list[CredentialMetadataConfiguration]: + """ + Return the active configurations to generate, narrowed to `fields`. + + Args: + fields (list of str | None): the fields to generate, or None for + every active configuration + + Returns: + list of CredentialMetadataConfiguration: the configurations to run + """ + configs = CredentialMetadataConfiguration.objects.filter(is_active=True) + if fields is not None: + configs = configs.filter(field__in=fields) + return list(configs) + + async def generate_credential_metadata( - resource: LearningResource, user=None + resource: LearningResource, user=None, fields: list[str] | None = None ) -> CredentialMetadata: """ - Generate every configured credential metadata field for a resource. + Generate configured credential metadata fields for a resource. The fields share one context and are independent, so they are generated concurrently: run in sequence they take about as long as the sum of their @@ -429,33 +474,45 @@ async def generate_credential_metadata( Args: resource (LearningResource): the resource to generate metadata for user (User): the user the generation is logged against + fields (list of str): the fields to generate, defaulting to every + active configuration. A caller filling in what a partial row is + missing passes that subset, so the fields already stored are + neither billed for a second time nor overwritten Returns: CredentialMetadata: the generated fields -- description (str) and - criteria (list[str]) -- and one error per configured field that is + criteria (list[str]) -- and one error per requested field that is missing from them. A field with no active configuration appears in neither: nothing was asked of it, so there is nothing to explain. """ - configs = await db_sync_to_async( - lambda: list(CredentialMetadataConfiguration.objects.filter(is_active=True)) - )() + configs = await db_sync_to_async(_active_configs)(fields) if not configs: logger.warning( - "No active CredentialMetadataConfiguration; nothing to generate for %s", + "No active CredentialMetadataConfiguration%s; nothing to generate for %s", + f" for {', '.join(fields)}" if fields is not None else "", resource.readable_id, ) return CredentialMetadata(fields={}, errors={}) - context = await build_credential_context(resource, retrieval_query(configs)) + query = retrieval_query(configs) + context = await build_credential_context(resource, query) + missing = _missing_context_sources(context, retrieved=bool(query)) + if missing: + sources = " and ".join(missing) + logger.warning( + "Not generating credential metadata for %s: missing its %s", + resource.readable_id, + sources, + ) + detail = f"Nothing was generated: the course is missing its {sources}." + return CredentialMetadata( + fields={}, errors={config.field: detail for config in configs} + ) + outcomes = await asyncio.gather( *[_generate_field(resource, config, context, user=user) for config in configs] ) - # Each response schema keys its value by the field name, so no field needs - # a case of its own here. An empty value is left out entirely, like a - # failure: a caller prepopulating a form must not overwrite a good value - # with a blank one -- and every field left out says why, so that a caller - # can tell a failed generation from one that produced nothing. fields, errors = {}, {} for config, outcome in zip(configs, outcomes): value = (outcome.response or {}).get(config.field) @@ -468,3 +525,25 @@ async def generate_credential_metadata( else: errors[config.field] = f"The model returned no {config.field}." return CredentialMetadata(fields=fields, errors=errors) + + +async def generate_and_save_credential_metadata( + resource: LearningResource, user=None, fields: list[str] | None = None +) -> CredentialMetadata: + """ + Generate a resource's credential metadata and store what was generated. + + Args: + resource (LearningResource): the resource to generate metadata for + user (User): the user the generation is logged against + fields (list of str): the fields to generate, defaulting to every + active configuration -- see `generate_credential_metadata` + + Returns: + CredentialMetadata: exactly what `generate_credential_metadata` + returned. Nothing is stored when it generated nothing, so a failed + run leaves the previous values in force. + """ + generated = await generate_credential_metadata(resource, user=user, fields=fields) + await db_sync_to_async(save_credential_metadata)(resource, generated.fields) + return generated diff --git a/learning_resources/credentials_store.py b/learning_resources/credentials_store.py new file mode 100644 index 0000000000..9e9816cecf --- /dev/null +++ b/learning_resources/credentials_store.py @@ -0,0 +1,162 @@ +""" +Read and write the credential metadata currently in force for a resource. +""" + +import logging + +from django.db.models import Q + +from learning_resources.models import ( + CredentialMetadata, + CredentialMetadataConfiguration, + LearningResource, +) + +logger = logging.getLogger(__name__) + + +def storable_credential_metadata_fields() -> set[str]: + """ + Return the CredentialMetadata columns a generated field can be written to. + + Returns: + set of str: the model's own field names + """ + return {field.name for field in CredentialMetadata._meta.concrete_fields} # noqa: SLF001 + + +def active_credential_metadata_fields() -> list[str]: + """ + Return the stored fields a generation would currently produce. + + Generation runs only is_active configurations, and a field with none is + left out of both the generated fields and the errors -- nothing was asked + of it, so there is nothing to explain. Its column therefore keeps its + default however many times the resource is generated for. Anything + deciding whether a resource still needs generating has to ask what is + configured, not what the model has columns for: a predicate that always + demanded every column would requeue every affected course on every sweep, + paying to regenerate the fields that are still active each time. + + Returns: + list of str: the active configurations' fields that have a column to + store them in, sorted. Empty means a generation would produce + nothing at all. + """ + configured = ( + CredentialMetadataConfiguration.objects.filter(is_active=True) + .values_list("field", flat=True) + .distinct() + ) + storable = storable_credential_metadata_fields() + return sorted(field for field in configured if field in storable) + + +def _empty_value(field: str): + """Return the stored field's own default, which is what "missing" means.""" + return CredentialMetadata._meta.get_field(field).get_default() # noqa: SLF001 + + +def incomplete_credential_metadata_query(fields: list[str]) -> Q: + """ + Return a LearningResource filter for metadata missing any of `fields`. + + "Missing" is per field and taken from the column's own default, so adding + a field needs no case here. + + Args: + fields (list of str): the stored fields that must be present, from + active_credential_metadata_fields() + + Returns: + Q: matches a resource with no metadata row at all, or one whose row + still holds the default for one of `fields` + """ + query = Q(credential_metadata__isnull=True) + for field in fields: + query |= Q(**{f"credential_metadata__{field}": _empty_value(field)}) + return query + + +def missing_credential_metadata_fields( + resource: LearningResource, fields: list[str] +) -> list[str]: + """ + Return which of `fields` the resource has no stored value for. + + + Args: + resource (LearningResource): the resource to look up + fields (list of str): the stored fields to check, from + active_credential_metadata_fields() + + Returns: + list of str: the subset of `fields` still holding the column default, + in the order given. Every field when the resource has no metadata + row at all. + """ + stored = stored_credential_metadata(resource) + if not stored: + return list(fields) + return [field for field in fields if getattr(stored, field) == _empty_value(field)] + + +def stored_credential_metadata( + resource: LearningResource, +) -> CredentialMetadata | None: + """ + Return the resource's stored credential metadata, or None if it has none. + + Args: + resource (LearningResource): the resource to look up + + Returns: + CredentialMetadata | None: the stored row, or None when nothing has + been generated for the resource yet + """ + return CredentialMetadata.objects.filter(learning_resource=resource).first() + + +def save_credential_metadata( + resource: LearningResource, fields: dict +) -> CredentialMetadata | None: + """ + Store the generated credential metadata fields for a resource. + + Only the fields actually generated are written. A field that failed is + left alone rather than blanked, so a partial generation cannot destroy a + good value from an earlier run -- the same omit-empties rule + `generate_credential_metadata` applies to its own return value. + + Args: + resource (LearningResource): the resource the metadata belongs to + fields (dict): the generated fields, keyed by CredentialMetadataField + name. Unknown keys are ignored. + + Returns: + CredentialMetadata | None: the stored row, or None when there was + nothing to store. Nothing is written for empty `fields`: an empty + row would read as "generated nothing" and be skipped by every + later sweep, so one provider outage would permanently poison the + resources it hit. + """ + storable = storable_credential_metadata_fields() + unknown = set(fields) - storable + if unknown: + logger.warning( + "Ignoring credential metadata field(s) %s for %s: not stored fields", + ", ".join(sorted(unknown)), + resource.readable_id, + ) + # Whitelisted, not passed through: `fields` keys come from + # CredentialMetadataField, and adding a member there without a column + # would otherwise raise FieldError on every resource in the sweep. + defaults = { + key: value for key, value in fields.items() if key in storable and value + } + if not defaults: + return None + stored, _ = CredentialMetadata.objects.update_or_create( + learning_resource=resource, defaults=defaults + ) + return stored diff --git a/learning_resources/credentials_store_test.py b/learning_resources/credentials_store_test.py new file mode 100644 index 0000000000..0ffc8ca4e1 --- /dev/null +++ b/learning_resources/credentials_store_test.py @@ -0,0 +1,248 @@ +"""Tests for reading and writing stored credential metadata""" + +import pytest + +from learning_resources.constants import CredentialMetadataField +from learning_resources.credentials_store import ( + active_credential_metadata_fields, + incomplete_credential_metadata_query, + missing_credential_metadata_fields, + save_credential_metadata, + stored_credential_metadata, +) +from learning_resources.factories import ( + CredentialMetadataConfigurationFactory, + CredentialMetadataFactory, + LearningResourceFactory, +) +from learning_resources.models import ( + CredentialMetadata, + CredentialMetadataConfiguration, + LearningResource, +) + +pytestmark = pytest.mark.django_db + + +@pytest.fixture +def resource(): + """Return a course to store metadata against""" + return LearningResourceFactory.create(is_course=True) + + +def test_stored_credential_metadata_none(resource): + """A resource with no metadata reads as None, not an exception""" + assert stored_credential_metadata(resource) is None + + +def test_stored_credential_metadata(resource): + """A stored row is read back""" + stored = CredentialMetadataFactory.create(learning_resource=resource) + + assert stored_credential_metadata(resource) == stored + + +def test_save_credential_metadata_creates(resource): + """Generated fields are stored""" + saved = save_credential_metadata( + resource, {"description": "A course.", "criteria": ["Did a thing"]} + ) + + assert saved.learning_resource == resource + assert saved.description == "A course." + assert saved.criteria == ["Did a thing"] + + +def test_save_credential_metadata_updates(resource): + """A second save replaces the stored values rather than adding a row""" + save_credential_metadata(resource, {"description": "First", "criteria": ["One"]}) + save_credential_metadata(resource, {"description": "Second", "criteria": ["Two"]}) + + stored = stored_credential_metadata(resource) + assert CredentialMetadata.objects.filter(learning_resource=resource).count() == 1 + assert stored.description == "Second" + assert stored.criteria == ["Two"] + + +def test_save_credential_metadata_keeps_a_field_that_failed(resource): + """ + A field missing from a later generation keeps its previous value. + + The generator omits a field it could not produce, so writing the whole + model every time would let one failed field blank a good value. + """ + save_credential_metadata(resource, {"description": "Good", "criteria": ["Good"]}) + save_credential_metadata(resource, {"description": "Regenerated"}) + + stored = stored_credential_metadata(resource) + assert stored.description == "Regenerated" + assert stored.criteria == ["Good"] + + +@pytest.mark.parametrize( + "fields", [{}, {"description": "", "criteria": []}, {"criteria": []}] +) +def test_save_credential_metadata_writes_nothing_when_empty(resource, fields): + """ + A generation that produced nothing stores nothing. + + """ + assert save_credential_metadata(resource, fields) is None + assert not CredentialMetadata.objects.filter(learning_resource=resource).exists() + + +def test_save_credential_metadata_ignores_an_unknown_field(resource): + """ + A field with no column is dropped, not passed to the ORM. + + The keys come from CredentialMetadataField; adding a member there without + a migration would otherwise raise FieldError on every resource in a sweep. + """ + saved = save_credential_metadata( + resource, {"description": "A course.", "alignment": ["Some standard"]} + ) + + assert saved.description == "A course." + assert not hasattr(saved, "alignment") + + +@pytest.fixture +def configurations(): + """ + One active configuration per credential metadata field. + + Created here rather than relying on migration 0124's seed, which a + transactional test elsewhere deletes without restoring. + """ + CredentialMetadataConfiguration.objects.all().delete() + return [ + CredentialMetadataConfigurationFactory.create(field=field.name) + for field in CredentialMetadataField + ] + + +def test_active_credential_metadata_fields(configurations): + """Every active configuration's field is reported""" + assert active_credential_metadata_fields() == sorted( + field.name for field in CredentialMetadataField + ) + + +def test_active_credential_metadata_fields_skips_inactive(configurations): + """A field whose configuration is switched off is not reported""" + CredentialMetadataConfiguration.objects.filter( + field=CredentialMetadataField.criteria.name + ).update(is_active=False) + + assert active_credential_metadata_fields() == [ + CredentialMetadataField.description.name + ] + + +def test_active_credential_metadata_fields_with_none_active(configurations): + """No active configuration means a generation would produce nothing""" + CredentialMetadataConfiguration.objects.update(is_active=False) + + assert active_credential_metadata_fields() == [] + + +def test_active_credential_metadata_fields_skips_a_field_with_no_column( + configurations, +): + """ + A configured field with nowhere to store it is not reported. + + Same whitelist as the writer: adding a CredentialMetadataField member + without a migration must not produce a filter on a column that does not + exist. + """ + CredentialMetadataConfiguration.objects.filter( + field=CredentialMetadataField.criteria.name + ).update(field="alignment") + + assert active_credential_metadata_fields() == [ + CredentialMetadataField.description.name + ] + + +@pytest.mark.parametrize( + ("stored", "fields", "expected"), + [ + (None, ["description", "criteria"], True), + ( + {"description": "A course", "criteria": ["Did a thing"]}, + ["description", "criteria"], + False, + ), + ( + {"description": "A course", "criteria": []}, + ["description", "criteria"], + True, + ), + ({"description": "A course", "criteria": []}, ["description"], False), + ({"description": "A course", "criteria": []}, ["criteria"], True), + ({"description": "", "criteria": ["Did a thing"]}, ["description"], True), + ({"description": "", "criteria": ["Did a thing"]}, ["criteria"], False), + ], +) +def test_incomplete_credential_metadata_query(resource, stored, fields, expected): + """ + The filter matches a resource missing any of the fields asked for. + + `stored` is None for a resource with no metadata row at all, which always + matches: nothing has been generated for it. + """ + if stored is not None: + CredentialMetadataFactory.create(learning_resource=resource, **stored) + + matches = ( + LearningResource.objects.filter(incomplete_credential_metadata_query(fields)) + .filter(id=resource.id) + .exists() + ) + + assert matches is expected + + +@pytest.mark.parametrize( + ("stored", "fields", "expected"), + [ + (None, ["criteria", "description"], ["criteria", "description"]), + ( + {"description": "A course", "criteria": ["Did a thing"]}, + ["criteria", "description"], + [], + ), + ( + {"description": "A course", "criteria": []}, + ["criteria", "description"], + ["criteria"], + ), + ( + {"description": "", "criteria": ["Did a thing"]}, + ["criteria", "description"], + ["description"], + ), + # Only what was asked for: a field with no active configuration is + # nobody's to generate, however empty its column is. + ({"description": "", "criteria": []}, ["criteria"], ["criteria"]), + ], +) +def test_missing_credential_metadata_fields(resource, stored, fields, expected): + """ + Only the fields still holding their column default come back. + + This is what scopes a regeneration: the fields left out are already in + force, and generating them again would both cost a call and replace them. + """ + if stored is not None: + CredentialMetadataFactory.create(learning_resource=resource, **stored) + + assert missing_credential_metadata_fields(resource, fields) == expected + + +def test_missing_credential_metadata_fields_without_active_configurations(resource): + """Nothing is configured, so nothing is missing -- there is nothing to ask for""" + CredentialMetadataFactory.create(learning_resource=resource, description="") + + assert missing_credential_metadata_fields(resource, []) == [] diff --git a/learning_resources/credentials_test.py b/learning_resources/credentials_test.py index 4c5cae13ec..c0a700f69e 100644 --- a/learning_resources/credentials_test.py +++ b/learning_resources/credentials_test.py @@ -2,6 +2,7 @@ import asyncio import json +import logging import pytest @@ -16,6 +17,7 @@ _get_llm, _prepare_marketing_page, build_credential_context, + generate_and_save_credential_metadata, generate_credential_metadata, ) from learning_resources.etl.constants import MARKETING_PAGE_FILE_TYPE @@ -25,10 +27,12 @@ LearningResourceFactory, ) from learning_resources.models import ( + CredentialMetadata, CredentialMetadataConfiguration, CredentialMetadataGenerationLog, ) from main.factories import UserFactory +from main.utils import run_on_worker_loop # Shaped like a real scraped program page: the program's own instructors come # before every child course's content, and the site footer trails the last @@ -637,3 +641,309 @@ def test_generate_credential_metadata_truncates_a_long_error( # The record keeps what the response cannot carry. log = CredentialMetadataGenerationLog.objects.get(learning_resource=resource) assert detail in log.error + + +@pytest.mark.django_db(transaction=True) +def test_generate_credential_metadata_for_a_subset_of_fields( + resource, configurations, mock_llm, mock_retrieval +): + """ + `fields` narrows generation to what was asked for. + + A resource whose description stored but whose criteria did not is + regenerated for criteria alone: the description already in force is + neither billed for again nor replaced. + """ + generated = asyncio.run(generate_credential_metadata(resource, fields=["criteria"])) + + assert set(generated.fields) == {"criteria"} + # Nothing is owed for a field nobody asked about. + assert generated.errors == {} + assert BadgeDescription not in mock_llm.prompts + assert BadgeCriteria in mock_llm.prompts + + +@pytest.mark.django_db(transaction=True) +def test_generating_only_a_description_skips_retrieval( + resource, configurations, mock_llm, mock_retrieval +): + """ + The retrieval query comes from the narrowed configurations. + + Only criteria reads course content, so a description-only run has nothing + to retrieve for and does not pay Qdrant for chunks no prompt will see. + """ + generated = asyncio.run( + generate_credential_metadata(resource, fields=["description"]) + ) + + assert set(generated.fields) == {"description"} + mock_retrieval.assert_not_called() + + +@pytest.mark.django_db(transaction=True) +def test_generate_credential_metadata_with_no_fields( + resource, configurations, mock_llm, mock_retrieval, caplog +): + """ + An empty `fields` generates nothing, rather than falling back to all. + + The caller asked for no field in particular -- a row that filled in + between being queued and being run -- which is not a licence to + regenerate the whole of it. + """ + with caplog.at_level(logging.WARNING): + generated = asyncio.run(generate_credential_metadata(resource, fields=[])) + + assert generated == ({}, {}) + assert mock_llm.prompts == {} + assert "No active CredentialMetadataConfiguration" in caplog.text + + +@pytest.mark.django_db(transaction=True) +def test_generate_and_save_credential_metadata_keeps_the_fields_not_asked_for( + resource, configurations, mock_llm, mock_retrieval +): + """ + A scoped regeneration leaves the stored fields it did not generate alone. + + Both fields are editable in the admin, so the description on a row whose + criteria never generated may have been corrected by hand. Regenerating + the row whole would replace it on the next sweep. + """ + CredentialMetadata.objects.create( + learning_resource=resource, + description="ORIGINAL REVIEWED DESCRIPTION", + criteria=[], + ) + + run_on_worker_loop( + generate_and_save_credential_metadata(resource, fields=["criteria"]) + ) + + stored = CredentialMetadata.objects.get(learning_resource=resource) + assert stored.description == "ORIGINAL REVIEWED DESCRIPTION" + assert stored.criteria == [ + "Applied conservation laws", + "Modelled fluid flow", + ] + + +@pytest.mark.django_db(transaction=True) +def test_generate_and_save_credential_metadata( + resource, configurations, mock_llm, mock_retrieval +): + """The generated fields are stored as well as returned""" + generated = run_on_worker_loop(generate_and_save_credential_metadata(resource)) + + stored = CredentialMetadata.objects.get(learning_resource=resource) + assert stored.description == generated.fields["description"] + assert stored.criteria == generated.fields["criteria"] + + +@pytest.mark.django_db(transaction=True) +def test_generate_and_save_stores_nothing_when_nothing_generated( + resource, no_configurations, mock_llm, mock_retrieval +): + """A run with nothing to generate leaves no row behind""" + generated = run_on_worker_loop(generate_and_save_credential_metadata(resource)) + + assert generated.fields == {} + assert not CredentialMetadata.objects.filter(learning_resource=resource).exists() + + +@pytest.mark.django_db(transaction=True) +def test_generate_and_save_over_several_resources_in_one_process( + resource, configurations, mock_llm, mock_retrieval +): + """ + A sweep generates with full context for every resource, not just the first. + + This is the regression test for the sync-to-async bridge. The clients + retrieval reaches are @cache'd and bound to the event loop that was alive + when they were built, so a loop per resource (asyncio.run, async_to_sync) + leaves resource two driving a cached channel onto a closed loop. + `_retrieve_chunks` catches that exception, after which generation is + skipped because required course content is missing. The assertions that + retrieval ran once per resource and that both rows were stored therefore + prove every resource used the live loop. + """ + second = LearningResourceFactory.create(is_course=True) + ContentFileFactory.create( + learning_resource=second, + file_type=MARKETING_PAGE_FILE_TYPE, + content=MARKETING_PAGE, + published=True, + ) + + for each in (resource, second): + run_on_worker_loop(generate_and_save_credential_metadata(each)) + + assert mock_retrieval.call_count == 2 + assert CredentialMetadata.objects.count() == 2 + + +@pytest.mark.django_db(transaction=True) +def test_generation_stops_without_a_marketing_page( + configurations, mock_llm, mock_retrieval, caplog +): + """ + A resource with no marketing page is not generated for. + + Its metadata alone would still produce plausible-looking output, which is + the problem: there is no way to tell it apart from a result with the + course behind it. + """ + resource = LearningResourceFactory.create(is_course=True) + + generated = asyncio.run(generate_credential_metadata(resource)) + + assert generated.fields == {} + assert set(generated.errors) == {field.name for field in CredentialMetadataField} + assert "missing its marketing page" in generated.errors["description"] + # No LLM call: an incomplete course costs nothing. + assert mock_llm.prompts == {} + assert "Not generating credential metadata" in caplog.text + + +@pytest.mark.django_db(transaction=True) +def test_generation_stops_without_course_content( + resource, configurations, mock_llm, mocker, caplog +): + """A resource with nothing indexed is not generated for""" + mocker.patch( + "learning_resources.credentials.async_content_file_chunks_for_resource", + return_value=[], + ) + + generated = asyncio.run(generate_credential_metadata(resource)) + + assert generated.fields == {} + assert "missing its course content" in generated.errors["criteria"] + assert mock_llm.prompts == {} + assert "Not generating credential metadata" in caplog.text + + +@pytest.mark.django_db(transaction=True) +def test_missing_context_is_a_warning_without_a_traceback( + configurations, mock_llm, mock_retrieval, caplog +): + """ + An incomplete course logs one warning, not an ERROR traceback. + + A course whose marketing page has not been scraped yet is an ordinary + state of the catalogue, and during a Qdrant outage the daily sweep would + otherwise print two contradictory tracebacks per affected course -- the + retrieval failure's, and one for a decision that has no traceback of its + own. + """ + resource = LearningResourceFactory.create(is_course=True) + + with caplog.at_level(logging.DEBUG, logger="learning_resources.credentials"): + asyncio.run(generate_credential_metadata(resource)) + + records = [ + record + for record in caplog.records + if "Not generating credential metadata" in record.message + ] + assert len(records) == 1 + assert records[0].levelno == logging.WARNING + assert records[0].exc_info is None + + +@pytest.mark.django_db(transaction=True) +def test_retrieval_failure_says_generation_is_skipped( + resource, configurations, mock_llm, mocker, caplog +): + """ + The retrieval failure log says what now actually happens. + + It used to promise generation from the metadata and marketing page alone, + which contradicted the abort logged right after it. + """ + mocker.patch( + "learning_resources.credentials.async_content_file_chunks_for_resource", + side_effect=ConnectionError("qdrant is down"), + ) + + asyncio.run(generate_credential_metadata(resource)) + + assert "skipping generation" in caplog.text + assert "marketing page alone" not in caplog.text + + +@pytest.mark.django_db(transaction=True) +def test_generation_stops_when_retrieval_fails( + resource, configurations, mock_llm, mocker +): + """ + A Qdrant failure stops generation rather than degrading it. + + This is the case that used to succeed quietly, and is what made a broken + event loop in the sweep invisible: criteria generated from marketing copy + with none of the course's content read the same as criteria with it. + """ + mocker.patch( + "learning_resources.credentials.async_content_file_chunks_for_resource", + side_effect=ConnectionError("qdrant is down"), + ) + + generated = asyncio.run(generate_credential_metadata(resource)) + + assert generated.fields == {} + assert "missing its course content" in generated.errors["criteria"] + assert mock_llm.prompts == {} + + +@pytest.mark.django_db(transaction=True) +def test_generation_names_every_missing_source( + configurations, mock_llm, mocker, caplog +): + """Both missing sources are named, so one fix does not hide the other""" + resource = LearningResourceFactory.create(is_course=True) + mocker.patch( + "learning_resources.credentials.async_content_file_chunks_for_resource", + return_value=[], + ) + + generated = asyncio.run(generate_credential_metadata(resource)) + + assert ( + "missing its marketing page and course content" + in generated.errors["description"] + ) + + +@pytest.mark.django_db(transaction=True) +def test_generation_without_retrieval_needs_no_content( + resource, no_configurations, mock_llm, mock_retrieval +): + """ + A blank retrieval_query still generates from the marketing page alone. + + Empty chunks are only a missing source when content was actually asked + for: a configuration with no query has said the marketing page is enough, + and demanding content would block that setup entirely. + """ + CredentialMetadataConfigurationFactory.create( + field=CredentialMetadataField.criteria.name, retrieval_query="" + ) + + generated = asyncio.run(generate_credential_metadata(resource)) + + mock_retrieval.assert_not_called() + assert generated.fields["criteria"] + assert generated.errors == {} + + +@pytest.mark.django_db(transaction=True) +def test_generate_and_save_stores_nothing_for_an_incomplete_course( + configurations, mock_llm, mock_retrieval +): + """An incomplete course is left with no stored metadata, so it is retried""" + resource = LearningResourceFactory.create(is_course=True) + + run_on_worker_loop(generate_and_save_credential_metadata(resource)) + + assert not CredentialMetadata.objects.filter(learning_resource=resource).exists() diff --git a/learning_resources/etl/edx_shared.py b/learning_resources/etl/edx_shared.py index 71fff4dbf3..4f940ec255 100644 --- a/learning_resources/etl/edx_shared.py +++ b/learning_resources/etl/edx_shared.py @@ -11,14 +11,15 @@ from django.core.cache import caches from django.db.models import Prefetch, Q +from learning_resources.constants import VALID_TEXT_FILE_TYPES from learning_resources.etl.constants import ETLSource from learning_resources.etl.loaders import load_content_files from learning_resources.etl.utils import ( calc_checksum, + excluded_olx_paths, get_bucket_by_name, get_edx_module_id, get_s3_prefix_for_source, - staff_only_olx_paths, transform_content_files, ) from learning_resources.models import ContentFile, LearningResourceRun @@ -369,32 +370,42 @@ def sync_edx_course_files( ) -def unpublish_staff_only_content_files( - etl_source: str, ids: list[int], keys: list[str] -) -> int: +def unpublish_excluded_content_files( + etl_source: str, ids: list[int], keys: list[str], *, dry_run: bool = False +) -> list[dict]: """ - Unpublish (and deindex) content files under staff-only OLX subtrees for the - runs matching the given archive keys, without re-extracting anything. + Unpublish (and deindex) content files the course does not use — staff-only + subtrees, asset manifests and unreferenced static files — for the runs + matching the given archive keys, without re-extracting anything. Args: etl_source(str): The edx ETL source ids(list of int): list of course ids to process keys(list[str]): list of S3 archive keys to search through + dry_run(bool): count the rows but leave them published and deindex nothing Returns: - int: number of content files unpublished + list of dict: a row per run whose archive excludes content files it has, + counting the excluded rows, the ones this call unpublished (or would + have, under dry_run) and the run's content files in total. Counts, + not paths, so the payload stays small enough to cross the celery + result backend for every run at once. """ from learning_resources_search import tasks as search_tasks from vector_search import tasks as vector_tasks bucket = get_bucket_by_name(settings.COURSE_ARCHIVE_BUCKET_NAME) run_lookup = build_run_lookup(etl_source, ids) - total = 0 + rows = [] for key in keys: matching_runs = run_lookup.get(extract_run_id_from_key(etl_source, key)) if not matching_runs: continue run = matching_runs[0] + if not ContentFile.objects.filter(run=run).exists(): + # a run with no content files has none to unpublish, and its archive + # is a download and an extract to find that out + continue with TemporaryDirectory() as tempdir: tarpath = Path(tempdir, key.rsplit("/", maxsplit=1)[-1]) bucket.download_file(key, tarpath) @@ -408,23 +419,55 @@ def unpublish_staff_only_content_files( if olx_path is None: continue try: - hidden_paths = staff_only_olx_paths(olx_path) + excluded_paths = excluded_olx_paths(olx_path) except ElementTree.ParseError: log.exception("Malformed OLX in %s, skipping", key) continue - hidden_keys = {get_edx_module_id(str(path), run) for path in hidden_paths} - if not hidden_keys: + ingestable = [ + path + for path in olx_path.rglob("*") + if path.is_file() + and path.suffix.lower() in VALID_TEXT_FILE_TYPES + and not any( + "draft" in part for part in path.relative_to(olx_path).parts[:-1] + ) + ] + # get_edx_module_id writes a space as an underscore, so "foo bar.pdf" + # and "foo_bar.pdf" are one row; it stays if either path is ingested + excluded_keys = { + get_edx_module_id(str(path), run) + for path in ingestable + if path in excluded_paths + } - { + get_edx_module_id(str(path), run) + for path in ingestable + if path not in excluded_paths + } + if not excluded_keys: continue # scoped to this run: keys embed the run_id, but never rely on that alone - hidden_files = ContentFile.objects.filter(run=run, key__in=hidden_keys) - unpublished = hidden_files.filter(published=True).update(published=False) - total += unpublished - log.info( - "Unpublished %d staff-only content files for %s", unpublished, run.run_id - ) - # dispatched whenever hidden rows exist, not only when this call flipped - # them, so a re-run after a failed deindex task cleans up the indexes - if hidden_files.exists(): + excluded_files = ContentFile.objects.filter(run=run, key__in=excluded_keys) + excluded = excluded_files.count() + if not excluded: + continue + if dry_run: + unpublished = excluded_files.filter(published=True).count() + else: + unpublished = excluded_files.filter(published=True).update(published=False) + log.info( + "Unpublished %d excluded content files for %s", unpublished, run.run_id + ) + # dispatched whenever excluded rows exist, not only when this call + # flipped them, so a re-run after a failed deindex task cleans up + # the indexes search_tasks.deindex_run_content_files.delay(run.id, unpublished_only=True) vector_tasks.remove_unpublished_run_content_files.delay(run.id) - return total + rows.append( + { + "run_id": run.run_id, + "excluded": excluded, + "unpublished": unpublished, + "total": ContentFile.objects.filter(run=run).count(), + } + ) + return rows diff --git a/learning_resources/etl/edx_shared_test.py b/learning_resources/etl/edx_shared_test.py index 1d9cc34aa6..f60a5f378e 100644 --- a/learning_resources/etl/edx_shared_test.py +++ b/learning_resources/etl/edx_shared_test.py @@ -19,7 +19,7 @@ normalize_run_id, process_course_archive, sync_edx_course_files, - unpublish_staff_only_content_files, + unpublish_excluded_content_files, ) from learning_resources.etl.utils import get_edx_module_id, get_s3_prefix_for_source from learning_resources.factories import ( @@ -1660,7 +1660,9 @@ def _staff_only_archive(tmp_path) -> Path: "course/run.xml": '', "chapter/ok.xml": '', "sequential/seq_ok.xml": '', - "vertical/v_ok.xml": '', + "vertical/v_ok.xml": ( + '' + ), "html/h_ok.xml": '', "html/h_ok.html": "

ok

", "chapter/staff.xml": ( @@ -1670,6 +1672,13 @@ def _staff_only_archive(tmp_path) -> Path: "vertical/v_staff.xml": '', "html/h_staff.xml": '', "html/h_staff.html": "

staff

", + # nothing links this, so it is from an earlier offering + "static/stale_syllabus.pdf": "stale", + # the video id keeps subs_ABC123; its space-spelled twin is a stale copy + # that get_edx_module_id folds onto the same content file key + "video/vid.xml": '