Release 0.77.4 - #3769
Open
odlbot wants to merge 20 commits into
Open
Conversation
Three required-field additions in the regenerated client are breaking: - `ManagerEnrollmentCode` gains `email_status` and `email_status_event_timestamp` - `OrganizationPage` gains `sso_organization_id: string | null` - `b2bManagerOrganizationsList` gains a positional `sso_organization_id` parameter ahead of `options` (our only caller passes no arguments, so it is unaffected) Everything else in the release is additive and currently unused: `OrderStatus`/`ordersStatusRetrieve`, `b2bServiceOrganizationManagerCheck`, `EmailStatusEnum`, `anonymous_id` on baskets, and optional `LegalAddress` fields. `Basket.user`/`BasketWithProduct.user` also loosened to `number | null | undefined`; nothing reads `.user`. Give the two factories the new fields. `OrganizationCards.test.tsx` built eleven `OrganizationPage` literals by hand; route them through the factory the file already uses elsewhere so the next required field does not break them again. Every field except `sso_organization_id` was already explicitly overridden and `mergeOverrides` overwrites arrays rather than merging, so the objects are unchanged. Drop the `Analytics/orgUuid.ts` shim, which carried a TODO to delete itself once this client landed, and read `org.sso_organization_id` directly. Behavior is unchanged: all four analytics queries are gated on `enabled: analyticsAvailable`, which requires a truthy `orgUuid`, so an absent, null, or empty UUID still reads as "analytics unavailable" and never reaches the API with `undefined` in the path. Pin the version exactly rather than with a caret, matching `@mitodl/mit-learn-api-axios` and `@mitodl/smoot-design`. A caret on a date-versioned generated client lets a plain `yarn install` pull one that adds required fields, which is exactly the breakage above. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
* adding checks for ovs webhook links * adding tests * default OVS_ALLOWED_MEDIA_HOSTS to empty list * fix test and fix edge case * adding example to backend env
Bumps [pbkdf2](https://github.com/browserify/pbkdf2) from 3.1.2 to 3.1.6. - [Changelog](https://github.com/browserify/pbkdf2/blob/master/CHANGELOG.md) - [Commits](browserify/pbkdf2@v3.1.2...v3.1.6) --- updated-dependencies: - dependency-name: pbkdf2 dependency-version: 3.1.6 dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [protobufjs](https://github.com/protobufjs/protobuf.js) from 7.5.4 to 7.6.5. - [Release notes](https://github.com/protobufjs/protobuf.js/releases) - [Changelog](https://github.com/protobufjs/protobuf.js/blob/protobufjs-v7.6.5/CHANGELOG.md) - [Commits](protobufjs/protobuf.js@protobufjs-v7.5.4...protobufjs-v7.6.5) --- updated-dependencies: - dependency-name: protobufjs dependency-version: 7.6.5 dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [cipher-base](https://github.com/crypto-browserify/cipher-base) from 1.0.4 to 1.0.7. - [Changelog](https://github.com/browserify/cipher-base/blob/master/CHANGELOG.md) - [Commits](browserify/cipher-base@v1.0.4...v1.0.7) --- updated-dependencies: - dependency-name: cipher-base dependency-version: 1.0.7 dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…d X-Forwarded-For (#3751) * fix(content_feedback): key anonymous throttle on trusted client IP
* fix: add navbar for games --------- Co-authored-by: Ahtesham Quraish <ahtesham.quraish@192.168.1.62>
* fix: bump the hacksnack version * drop the local hacksnack resolution override The override pinned @mitodl/hacksnack to a git branch for local development, which conflicts with the lockfile's npm resolution and fails any immutable install. The package is consumed from npm. --------- Co-authored-by: Ahtesham Quraish <ahtesham.quraish@192.168.1.62> Co-authored-by: Ahtesham Quraish <ahtesham.quraish@arbisoft.com>
* feat: declare the resource canonical on news and article pages
/news/{article}?resource=<id> and /articles/{article}?resource=<id> now
emit the resource's canonical URL and social metadata, matching what /,
/search, and /c/ already do.
Also fixes getMetadataAsync pairing the resource's image with the caller's
alt text, and vice versa. Both branches were unreachable until now: these
are the first pages to pass an imageAlt.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* feat(ol-components): replace Link's shallow prop with pushUrl
shallow meant 'preventDefault and pushState the href', and required the
href to be ?-relative. pushUrl states the pushed URL instead, which lets a
link point at a real, crawlable URL while a click updates the current page.
shallow was the special case pushUrl === href.
Unlike shallow, pushUrl skips modified clicks, so this changes behaviour
everywhere shallow was in use — not just at the two Ask TIM callsites that
migrate explicitly. Linkable still infers pushUrl for ?-relative hrefs,
which is every resource card, so cmd/ctrl-clicking a card *title* now opens
a new tab instead of being swallowed. That makes the title agree with the
card body, which has always opened a new tab. Destinations are unchanged
until cards get canonical hrefs.
useClickChildLink forwarded only cmd/ctrl to the card link, so shift and
alt on the card *body* fell through to anchor.click(), which synthesizes an
unmodified click — a shift-click on a card body silently behaved as a plain
one. It now forwards all four, so body and title agree on every modifier
rather than only on cmd/ctrl. Middle-click on the body still does nothing;
that needs an auxclick handler, which is a larger change.
Card titles accept an explicit pushUrl and forward it, but no card passes
one yet.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* feat: resource cards link to the canonical resource URL
The href is now /search?resource=<id>&resource_title=<slug> on every page,
so cards stop generating one crawlable URL per host page per query-string
variant. A plain click still opens the drawer in place, via LinkAdapter's
pushUrl, which now carries the host page's params explicitly rather than
being inferred from a ?-relative href.
Also makes the href a pure function of the resource. It was previously
derived at render time from useSearchParams and window.location.hash, which
was a latent hydration mismatch on any page with a fragment.
Modified clicks now land on the canonical URL rather than the host page.
Cmd-clicking a card body previously opened the host page with its filters
intact, and that is a deliberate regression: the same href cannot be both
the crawlable URL and a carrier of host page state.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix: Ask TIM's drawer link carries resource_title
It inherited the current query string, so on a page that already had a
resource_title it paired that slug with a different resource id.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* refactor: let pushUrl be a function, evaluated at click time
`useResourceDrawerPushUrl` needed `useSearchParams()` to build the drawer
URL during render. That is a dynamic API: any client component calling it
forces its page to render dynamically or sit behind a Suspense boundary.
Resource cards render on nearly every page, so a value that only the click
handler ever reads was constraining how those pages can be rendered. Moot
today under the root `force-dynamic`, but it forecloses static/ISR — which
would beat link canonicalization on origin load, the point of this PR.
`pushUrl` now accepts `string | (() => string)`. The function form runs in
the click handler, so `resourceDrawerPushUrl` can read `window.location`
directly and is a plain function rather than a hook. Existing string
callers — both Ask TIM links — are unaffected; the widening is additive.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(ol-components): restore the same-page invariant pushUrl dropped
`shallow` asserted at render that its href started with "?", which was
the whole guarantee that a push only changed the query string. Splitting
it into `pushUrl` dropped that assertion, and the discriminated union
can't express it.
The constraint still holds. Next patches `window.history.pushState` to
dispatch ACTION_RESTORE carrying the *current* history entry's router
tree, so an external push swaps the canonical URL without re-rendering
the route: a cross-page pushUrl would leave the address bar describing a
page that is not mounted. This PR makes that mistake newly reachable,
since `href` is now the canonical cross-page URL and `pushUrl={href}` is
also the legitimate shallow-routing spelling.
Checked at click time rather than render, since a function pushUrl has no
value until then, and a failure falls through to navigating `href` rather
than throwing in a released build — the click degrades to a full
navigation to a correct URL instead of doing nothing.
Also drops the function-form prefetch test: `prefetch={pushUrl ? false :
undefined}` doesn't discriminate on type, so it retested the string form's
branch.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
…e summary endpoint (#3758) * Add resource_type and canonical_parent_ids to the summary endpoint The podcast and video sitemaps can't use /api/v1/learning_resources/summary/ because it doesn't carry enough to build their nested URLs, so they page the full list endpoint instead -- which caps limit at 100 while they request 1000, silently dropping ~87% of their entries (mitodl/mit-learn#3756). This adds what those sitemaps need so they can move over; switching them is follow-up work. - resource_type, so a caller can tell a podcast from an episode. Added to the queryset's .only() as well, or it would be deferred into a query per row. - canonical_parent_ids, the parents that form part of a resource's URL: parent podcasts for an episode, playlists for a video. Empty for everything else -- program and learning-path membership don't appear in a URL, and a learning path is a user-created list that must never leak here. Keeping the endpoint fast was the constraint, since it also serves the catalog-wide resources sitemap (12.4k published resources over 13 shards). canonical_parent_ids is a correlated ArraySubquery: one child_id index scan per row scanned, and no GROUP BY on the outer query. At production scale that is +0.8ms on a 1000-row page at offset 0 and +21ms at offset 12000, since OFFSET discards rows after the subplan has run, so deep pages pay for what they skip. A full 13-shard walk costs on the order of 100ms of extra database time. The pagination count needed handling. Django keeps annotations in the count query when the queryset is .distinct(), so left alone the subquery would run for every row counted rather than only the page -- 17ms against 3ms before this change. SummaryPagination counts distinct pks instead. That is the same number because get_aggregation clears the ordering before the DISTINCT, leaving only the .only() columns, which are all pk-dependent; it is also cheaper than the pre-change count, which hashed a six-column tuple rather than a bare pk. A test pins that the page query carries the annotation and the count query does not. The action is also now cached for anonymous users, like list already was, and ETL's clear_views_cache() invalidates it -- so the page costs land on a miss rather than every crawl. A cache hit is ~48x faster than a miss, mostly because it skips DRF serialization rather than the query. The decorator deliberately omits its timeout argument so REDIS_VIEW_CACHE_DURATION resolves per request instead of at import, which is what makes setting it to 0 actually disable the cache; every other call site in the repo freezes it at import. The detail endpoint's parent prefetches relied on LearningResourceRelationship.Meta.ordering rather than ordering explicitly. They and the new annotation now all name RELATIONSHIP_ORDERING, so the two endpoints agree by construction instead of by inheritance. This is defensive -- Meta.ordering already supplied the same order, and no test can distinguish the two. What is pinned is the agreement itself: a test asserts both endpoints report the same parents in the same order, using positions that are the reverse of creation order so an id-ordered annotation fails it. That agreement matters for 138 of 6,829 videos, the only resources in production with more than one canonical parent. Any member playlist is self-canonical, so a desync wouldn't produce a dead URL -- it would point the sitemap at a different playlist than the bare /video/<id> redirect uses. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Say why the count uses .values() rather than .only() The base DefaultPagination.get_count uses .only(), which defers concrete columns but leaves annotations in the SELECT list. Naming that at the call site, since the override otherwise reads as a gratuitous difference. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Drop the response cache from the summary action The endpoint served the resources and products sitemaps uncached at this scale already; the cache was added here on spec, not to fix a measured problem. An uncached 1000-row page is ~24ms, so a full 13-shard crawl costs about a third of a second. What it cost instead: parent ids go stale whenever an ETL path writes memberships without calling clear_views_cache() -- the YouTube tasks don't -- and cache keys hash the full query string, so any unknown param mints another ~247KB entry. Easy to add back with a deliberate invalidation story if something chattier than a crawler ever points at this. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
OpenAPI Changes4 changes: 2 error, 0 warning, 2 info Unexpected changes? Ensure your branch is up-to-date with |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
Comment on lines
+588
to
+598
| # only a full run knows the complete set of configured channels; a run | ||
| # filtered to specific channels must not unpublish the rest | ||
| loaders.unpublish_removed_youtube_channels( | ||
| [channel_config["channel_id"] for channel_config in channel_configs] | ||
| ) | ||
|
|
||
| log.info("Queueing %d youtube channels", len(channel_configs)) | ||
| for channel_config in channel_configs: | ||
| get_youtube_channel_data.delay(channel_config) | ||
|
|
||
| return len(channel_configs) |
There was a problem hiding this comment.
Bug: The YouTube ETL refactor removed cache invalidation, but no new mechanism was added for the asynchronous tasks. This will lead to serving stale data from the cache.
Severity: MEDIUM
Suggested Fix
Implement a mechanism to invalidate the cache after all the YouTube fan-out tasks have completed. This could be achieved using a Celery callback or a 'chord' that executes clear_views_cache() once all the parallel playlist/channel processing tasks are finished.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: learning_resources/tasks.py#L571-L598
Potential issue: The refactoring of the YouTube ETL process to use asynchronous fan-out
tasks removed the synchronous call to `clear_views_cache()`. While the main task
`get_youtube_data` no longer has anything to invalidate immediately, no alternative
mechanism was introduced to clear the cache after the asynchronous writes from the
fan-out tasks complete. This results in the view cache for YouTube resources never being
invalidated after an ETL run. Consequently, stale data will be served from the API until
the cache is manually cleared or otherwise expires, which is a functional regression
from the previous behavior.
Did we get this right? 👍 / 👎 to inform future reviews.
odlbot
added
waiting for checkboxes
and removed
deploying to rc
waiting for checkboxes
labels
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
8 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Chris Chudzicki
Matt Bertrand
renovate[bot]
Ahtesham Quraish
Zaman Afzal
dependabot[bot]
Anastasia Beglova
Shankar Ambady
Danielle Frappier