Add submission details page by AlexVelezLl · Pull Request #5716 · learningequality/studio

AlexVelezLl · 2026-02-23T16:25:29Z

Summary

Add submission details page.
Links the Notifications Page to the submission details page.
Links the admin channels table to the submission details page in review mode.

Grabacion.de.pantalla.2026-03-03.a.la.s.4.09.09.p.m.mov

References

Closes #5455.

Reviewer guidance

Create/reject/approve Community Library submissions for several channels/versions.
Go to the notifications page and click on the "view more" button of any notification.
Additionally, go to the admin channels table and click on a community library status button.
Review the submission if its status is pending.

...ration/frontend/shared/views/communityLibrary/SubmissionDetailsModal/ExpandableContainer.vue

rtibblesbot

Submission details modal — solid feature addition with a few issues to address.

CI: frontend tests, linting, and build pass. Python unit tests still pending.

blocking (2): modal-open class cleanup regression in StudioImmersiveModal will leave the page unscrollable after navigating away; a.html scratch file accidentally committed at repo root
suggestion (5): null guard gaps in ChannelDetails, hardcoded i18n string, missing expanded prop declaration, fragile composable API
nitpick (1): typo in string key
praise (2): see inline comments

@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly

How was this generated?

Reviewed the pull request diff checking for:

Correctness: bugs, edge cases, undocumented behavior, resource leaks, hardcoded values
Design: unnecessary complexity, naming, readability, comment accuracy, redundant state
Architecture: duplicated concerns, minimal interfaces, composition over inheritance
Testing: behavior-based assertions, mocks only at hard boundaries, accurate coverage
Completeness: missing dependencies, unupdated usages, i18n, accessibility, security
Principles: DRY (same reason to change), SRP, Rule of Three (no premature abstraction)
Checked CI status and linked issue acceptance criteria
For UI changes: inspected screenshots for layout, visual completeness, and consistency

rtibblesbot · 2026-03-03T13:49:46Z

contentcuration/contentcuration/frontend/shared/views/StudioImmersiveModal.vue

+      value: {
+        handler(newValue) {
+          if (newValue) {
+            document.documentElement.classList.add('modal-open');


blocking: The refactor from mounted()/beforeDestroy to a value watcher lost the beforeDestroy cleanup. When this component is destroyed while value is true (e.g., navigating away from the modal route), modal-open stays on <html>, leaving overflow-y: hidden !important and making the destination page unscrollable.

The old code cleaned this up:

this.$once('hook:beforeDestroy', () => { document.documentElement.classList.remove('modal-open'); });

Suggested fix — add cleanup back to the existing $once block at line 95:

this.$once('hook:beforeDestroy', () => { document.documentElement.classList.remove('modal-open'); document.removeEventListener('keydown', handleKeyDown); });

Thanks, haven't thought about that edge case.

rtibblesbot · 2026-03-03T13:49:46Z

a.html

@@ -0,0 +1,167 @@
+<div class="disclosure-cards">


blocking: This 167-line HTML file ("Musical Education Conference" disclosure cards demo) appears to be an accidentally committed scratch file. It has no relation to the community library submission feature and sits at the repo root. Please remove it from the branch.

Whoooooooppsssss 😆

rtibblesbot · 2026-03-03T13:49:46Z

...entcuration/frontend/shared/views/communityLibrary/SubmissionDetailsModal/ChannelDetails.vue

+  });
+
+  const languagesString = computed(() => {
+    const languages = props.channelVersion.included_languages || [];


suggestion: props.channelVersion has default: null (line 128). If the ChannelVersion.fetchCollection call in the parent returns empty results, channelVersion will be undefined/null and this line throws TypeError: Cannot read properties of null.

Lines 229–230 and 236–237 use optional chaining (?.map) but this line does not. Similarly, the details computed (lines 192–196) and :channelVersionId="channelVersion.id" (line 39) access channelVersion without guards.

Does the backend guarantee a matching ChannelVersion always exists for every submission? If not, defensive guards are needed:

const languages = props.channelVersion?.included_languages || [];

Yes, ChannelVersion must exist for every submission. Changed the props declaration to declare this as required

rtibblesbot · 2026-03-03T13:49:46Z

...entcuration/frontend/shared/views/communityLibrary/SubmissionDetailsModal/ChannelDetails.vue

+  const licensesString = computed(() => {
+    return (
+      props.channelVersion.included_licenses
+        ?.map(licenseId => LicensesMap.get(licenseId).license_name)


suggestion: LicensesMap.get(licenseId) returns undefined for an unknown license ID, causing .license_name to throw. Same issue at line 221 with LanguagesMap.get(code).readable_name. Consider optional chaining:

LanguagesMap.get(code)?.readable_name LicensesMap.get(licenseId)?.license_name

Thanks, done!

rtibblesbot · 2026-03-03T13:49:46Z

...tion/contentcuration/frontend/shared/views/communityLibrary/SubmissionDetailsModal/index.vue

+        <div class="actions">
+          <KButton
+            v-if="adminReview && submission.status === CommunityLibraryStatus.PENDING"
+            text="review"


suggestion: text="review" is a hardcoded English string, bypassing the i18n system that every other label in this PR uses. Should this go through communityChannelsStrings or a similar translation source?

Great catch. This was initially not a problem because this will only get rendered to admins, but if in the future we enable other users to review other submissions, then this will be a problem. Fixed!

rtibblesbot · 2026-03-03T13:49:46Z

...ration/frontend/shared/views/communityLibrary/SubmissionDetailsModal/ExpandableContainer.vue

+
+  const emit = defineEmits(['open']);
+
+  defineProps({


suggestion: ChannelDetails.vue passes :expanded="true" to this component, but defineProps only declares title. The expanded prop is silently ignored and the container always starts collapsed (isOpen = ref(false)). If the intent is for Channel Details to start expanded, add the prop and wire it:

defineProps({ title: { type: String, default: '' }, expanded: { type: Boolean, default: false }, }); const isOpen = ref(props.expanded);

expanded is not expected to be a prop, just removed it. Thanks.

rtibblesbot · 2026-03-03T13:49:46Z

contentcuration/contentcuration/frontend/shared/composables/useSpecialPermissions.js

 *   helpers used by `SpecialPermissionsList.vue`.
 */
-export function useSpecialPermissions(channelVersionId) {
+export function useSpecialPermissions(channelVersionId, { distributable }) {


suggestion: Destructuring { distributable } without a default for the second argument means useSpecialPermissions(id) or useSpecialPermissions(id, undefined) will throw TypeError: Cannot destructure property 'distributable' of undefined. Consider:

export function useSpecialPermissions(channelVersionId, { distributable } = {}) {

Also, the distributable !== null guard at line 52 won't catch undefined — if distributable is omitted, it will be undefined and the filter will be set to undefined. Using distributable != null (loose equality) would handle both.

Thanks! Done :)

rtibblesbot · 2026-03-03T13:49:46Z

contentcuration/contentcuration/frontend/shared/strings/communityChannelsStrings.js

    message: 'Submitted',
    context: 'Status indicating that an Community Library submission is pending',
  },
+  superseededStatus: {


nitpick: The key superseededStatus has a spelling error — "superseded" has one 'e', not two. The message text is correct ('Superseded'), but the key name propagates the typo to CommunityLibraryStatusChip.vue. Easier to fix now than after it spreads to more consumers.

Great catch

rtibblesbot · 2026-03-03T13:49:46Z

...tion/contentcuration/frontend/shared/views/communityLibrary/SubmissionDetailsModal/index.vue

+          <div>
+            <h1
+              class="notranslate"
+              dir="auto"


praise: Good use of dir="auto" and class="notranslate" for user-generated channel names and descriptions. This correctly handles RTL content and prevents machine translation from mangling channel names.

rtibblesbot · 2026-03-03T13:49:46Z

...ntcuration/frontend/shared/views/communityLibrary/SubmissionDetailsModal/ActivityHistory.vue

+    margin: 16px 0 0;
+    list-style: none;
+
+    li {


praise: The timeline connector CSS using ::before/::after pseudo-elements is elegantly implemented — clean, no JS overhead, and visually effective.

rtibblesbot

Good progress — all 8 prior findings resolved.

CI: only the spreadsheet-update workflow is visible (passing). Core test/lint/build checks not shown.

blocking (1): activity history channel filter not applied — see inline
suggestion (1): ExpandableContainer event semantics
praise (2): see inline

@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly

How was this generated?

Compared the current PR state against findings from a prior review:

Retrieved prior bot reviews deterministically via the GitHub API
Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
Only raised NEW findings for newly introduced code
Reviewed the pull request diff checking for correctness, design, architecture, testing, completeness, and adherence to DRY/SRP principles
Checked CI status and linked issue acceptance criteria

rtibblesbot · 2026-03-03T14:31:20Z

...ntcuration/frontend/shared/views/communityLibrary/SubmissionDetailsModal/ActivityHistory.vue

+  });
+  const { hasMore, submissionsUpdates, isLoading, isLoadingMore, fetchData, fetchMore } =
+    useCommunityLibraryUpdates({
+      channel: props.channelId,


blocking: useCommunityLibraryUpdates destructures { queryParams } from its options object (composable line 55), so { channel: props.channelId } silently discards the channel key — queryParams ends up undefined.

Even if it were passed inside queryParams, the composable's fetchSubmissionsUpdates only extracts date_updated__lte, date_updated__gte, status__in, search, and max_results (composable lines 153-159). The channel field is never forwarded to CommunityLibrarySubmission.fetchCollection, so the API call returns submissions across all channels.

The backend FilterSet does support a channel filter (viewset line 60). To fix, the composable needs to accept and forward the channel parameter — e.g.:

export default function useCommunityLibraryUpdates({ queryParams, channel } = {}) { // ... params = pickBy({ channel, date_updated__lte: _params?.date_updated__lte, // ... }); }

Great catch, thanks.

rtibblesbot · 2026-03-03T14:31:20Z

...ration/frontend/shared/views/communityLibrary/SubmissionDetailsModal/ExpandableContainer.vue

+
+  async function toggleDisclosure() {
+    isOpen.value = !isOpen.value;
+    emit('open', isOpen.value);


suggestion: emit('open', isOpen.value) fires for both open and close. In ChannelDetails, @open="onOpen" runs onOpen on every toggle — the guard at line 253 prevents duplicate fetches, so this won't cause bugs, but the event name is misleading. Consider either renaming to toggle or only emitting when opening:

if (isOpen.value) { emit('open'); }

rtibblesbot · 2026-03-03T14:31:20Z

contentcuration/contentcuration/frontend/shared/views/details/StudioDetailsPanel.vue

@@ -455,6 +486,10 @@
        type: Boolean,


praise: The hideChannelHeader prop and the conditional v-if guards on each detail section (levels, tags, languages, etc.) cleanly allow StudioDetailsPanel to be reused for the submission details page without duplicating the component. Well done making the existing component flexible rather than creating a new one.

rtibblesbot · 2026-03-03T14:31:20Z

...ntcuration/frontend/shared/views/communityLibrary/SubmissionDetailsModal/ActivityHistory.vue

+    gap: 8px;
+  }
+
+  .activity-list {


praise: The timeline CSS using ::before/::after pseudo-elements with SCSS variables is well-structured and easy to follow. Good use of named constants for spacing values.

rtibblesbot

Good progress — all 10 prior findings resolved.

CI: spreadsheet-update workflow passing; core test/lint/build checks not shown.

suggestion (1): inconsistent language label string in ChannelDetails — see inline
praise (1): see inline

@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly

How was this generated?

Compared the current PR state against findings from a prior review:

Retrieved prior bot reviews deterministically via the GitHub API
Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
Only raised NEW findings for newly introduced code
Reviewed the pull request diff checking for correctness, design, architecture, testing, completeness, and adherence to DRY/SRP principles
Checked CI status and linked issue acceptance criteria

rtibblesbot · 2026-03-03T20:53:33Z

...entcuration/frontend/shared/views/communityLibrary/SubmissionDetailsModal/ChannelDetails.vue

+              :text="countriesString"
+            />
+            <StudioDetailsRow
+              :label="languageLabel$()"


suggestion: This uses languageLabel$() which resolves to "Language" (singular, from the pre-existing string at communityChannelsStrings.js:72). The sibling labels all use the plural/parenthesized forms — countryLabel$() → "Country(s)", licensesLabel$() → "License(s)". The PR already added languagesLabel with message "Language(s)" (line 217), so this should likely be languagesLabel$() for consistency.

- :label="languageLabel$()" + :label="languagesLabel$()"

rtibblesbot · 2026-03-03T20:53:33Z

contentcuration/contentcuration/frontend/shared/views/details/StudioDetailsPanel.vue

@@ -4,32 +4,33 @@
    :class="{ printing }"
    data-testid="details-panel"


praise: The systematic null-guarding of detail rows (v-if on levels, categories, includes, tags, languages, authors, providers, aggregators, etc.) plus the hideChannelHeader prop make StudioDetailsPanel cleanly reusable for older channel versions where these fields are unavailable. The includesPrintable and licensesPrintable defensive checks are a nice touch too.

rtibblesbot

Clean feature addition — well-structured submission details page with good component decomposition.

CI passing. 11 prior finding(s) resolved.

suggestion (1): stale queryParams capture in ActivityHistory — see inline

@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly

How was this generated?

Compared the current PR state against findings from a prior review:

Retrieved prior bot reviews deterministically via the GitHub API
Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
Only raised NEW findings for newly introduced code
Reviewed the pull request diff checking for correctness, design, architecture, testing, completeness, and adherence to DRY/SRP principles
Checked CI status and linked issue acceptance criteria

rtibblesbot · 2026-03-03T21:35:47Z

...ntcuration/frontend/shared/views/communityLibrary/SubmissionDetailsModal/ActivityHistory.vue

+  const { hasMore, submissionsUpdates, isLoading, isLoadingMore, fetchData, fetchMore } =
+    useCommunityLibraryUpdates({
+      queryParams: {
+        channel: props.channelId,


suggestion: props.channelId is captured by value into a plain object here, so the composable's queryParams.channel won't update if channelId changes later. The watcher at line 217 calls fetchData() on channelId changes, but the composable will still use the stale value since unref() on a plain object returns it as-is.

In practice this doesn't bite today because in-page navigation (goToLiveSubmission, goToSubmission) stays within the same channel. But if cross-channel navigation ever becomes possible, wrapping this in a computed would keep the composable in sync:

const { ... } = useCommunityLibraryUpdates({ queryParams: computed(() => ({ channel: props.channelId })), });

rtibblesbot

Clean feature addition — well-structured submission details page with good component decomposition and thorough null-guarding.

CI passing. 12 prior finding(s) resolved.

praise (1): see inline

@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly

How was this generated?

Compared the current PR state against findings from a prior review:

Retrieved prior bot reviews deterministically via the GitHub API
Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
Only raised NEW findings for newly introduced code
Reviewed the pull request diff checking for correctness, design, architecture, testing, completeness, and adherence to DRY/SRP principles
Checked CI status and linked issue acceptance criteria

rtibblesbot · 2026-03-03T22:15:52Z

...ration/frontend/shared/views/communityLibrary/SubmissionDetailsModal/ExpandableContainer.vue

+        v-if="expandable"
+        appearance="basic-link"
+        :aria-controls="expandableContentId"
+        :aria-expanded="isOpen.toString()"


praise: Clean ARIA disclosure pattern — aria-controls linked to the content id, aria-expanded reflecting state, and scrollIntoView on expand. Well done.

AlexVelezLl · 2026-03-03T20:40:20Z

.../contentcuration/frontend/administration/components/sidePanels/ReviewSubmissionSidePanel.vue

It seems that v-bind has some difficulties when the nodes their styles are applied to are removed from the DOM, and then reinserted again. (In this case the v-if=isModalVisible update made it appear). It seems that when the nodes are removed, the CSS variables are removed too, but if they are re-inserted, the CSS variables are not. So, just came back these colors to be declared in inline styles. And flagging that this is something that can happen with Vue 2.7's v-bind method.

AlexVelezLl · 2026-03-03T20:43:26Z

.../contentcuration/frontend/administration/components/sidePanels/ReviewSubmissionSidePanel.vue

+            // Do not emit close just yet, so that the component isn't unmounted
+            // this will keep the component live until the submit function finishes, allowing
+            // to keep communicating with the parent component, and in particular allowing the
+            // "change" event to be emitted. It also allows us to keep the working information
+            // on the component, and show the side panel in the same state if the user cancels
+            isModalVisible.value = true;


This was the best way I found to manage this. It is similar to what we were doing initially for the BUM side panels, where the side panel component was still live until the user rejected the "undo" modal.

AlexVelezLl · 2026-03-03T20:56:38Z

contentcuration/contentcuration/frontend/shared/utils/helpers.js

+export function getCommunityLibrarySubmissionDetailsUrl(channelId, submissionId) {
+  return `/channels/#/community-library/${channelId}/${submissionId}`;
+}


@rtibbles I have had several internal conflicts regarding whether to treat this new page as a page (i.e., with its defined path) or only as a modal opened by a query param. Since here we needed some required arguments, the submissionId and channelId, I have made it a page, but the problem is that this page needs to be opened from any plugin, because we can open this page from the NotificationsModal.

Because of this, I had to do this getCommunityLibrarySubmissionDetailsUrl to get a link to a page outside the current Vue app, similar to what we did for the my downloads redirection, but not sure if there is a better path forward, or if we should just treat this as a modal, and open this with query params.

rtibbles self-assigned this Feb 24, 2026

MisRob reviewed Feb 26, 2026

View reviewed changes

...ration/frontend/shared/views/communityLibrary/SubmissionDetailsModal/ExpandableContainer.vue Show resolved Hide resolved

AlexVelezLl force-pushed the new-channel-details branch from fbafc5c to e32b7f7 Compare February 27, 2026 17:16

AlexVelezLl added 7 commits March 3, 2026 08:30

Add page setup and channel info header

025f6b0

Add Channel details accordion

1eadd00

Add ActivityHistory component

c54fb2e

Add activity line

a060864

Polish submission details modal

034be9b

Display SubmissionDetailsModal on channel admin table

3e555fc

Fix StudioImmersiveModal always hiding html scroll

efa791b

AlexVelezLl force-pushed the new-channel-details branch from a74181e to 8eb02fe Compare March 3, 2026 13:40

AlexVelezLl marked this pull request as ready for review March 3, 2026 13:40

AlexVelezLl requested a review from rtibblesbot March 3, 2026 13:40

AlexVelezLl changed the title ~~Add submission details modal~~ Add submission details page Mar 3, 2026

rtibblesbot suggested changes Mar 3, 2026

View reviewed changes

AlexVelezLl added 3 commits March 3, 2026 09:10

Refactor and rename Accordion component

71cd097

Fix failing tests

410ced7

Store notifications tab on route

55bea6c

AlexVelezLl force-pushed the new-channel-details branch from 8eb02fe to 55bea6c Compare March 3, 2026 14:10

AlexVelezLl requested a review from rtibblesbot March 3, 2026 14:14

rtibblesbot suggested changes Mar 3, 2026

View reviewed changes

AlexVelezLl requested a review from rtibblesbot March 3, 2026 20:36

rtibblesbot reviewed Mar 3, 2026

View reviewed changes

AlexVelezLl force-pushed the new-channel-details branch from e060332 to b7013da Compare March 3, 2026 21:15

rtibblesbot reviewed Mar 3, 2026

View reviewed changes

Address PR comments

8cead6d

AlexVelezLl force-pushed the new-channel-details branch from b7013da to 8cead6d Compare March 3, 2026 21:56

rtibblesbot approved these changes Mar 3, 2026

View reviewed changes

AlexVelezLl commented Mar 3, 2026

View reviewed changes

		@@ -4,32 +4,33 @@
		:class="{ printing }"
		data-testid="details-panel"

Conversation

AlexVelezLl commented Feb 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

References

Reviewer guidance

Uh oh!

Uh oh!

rtibblesbot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rtibblesbot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rtibblesbot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rtibblesbot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rtibblesbot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

AlexVelezLl commented Feb 23, 2026 •

edited

Loading