Robust solution for filtering unpublishable changes on frontend

[AlexVelezLl]

Summary

In #5825, I noticed that unpublishable changes were still changing the unpublished_changes condition on the frontend because we weren't returning this from the backend, and less so acknowledging it on the frontend. I just followed the tendency to just remove the created_by_id attribute from the change, just to not mess with the sync endpoint.

However, didn't notice that in the add_to_community_library flow, we use get_edit_queryset to filter the channel, so we needed the created_by_id. This PR returns the unpublishable field to the frontend, and filters changes to compute the unpublished_changes field.

Verification steps

• I have gone through the whole flow of approving a Community Library Submission, and seeing the channel live on the Community Library.
• No unwanted enabled publish button after that.
• Verified that other publishable sync changes from other browsers keep working correctly and setting the unpublished_changes field properly.

References

Fixes #5843.

Reviewer guidance

• Create a community library submission.
• Approve it.
• Check that the channel is live on the community library page.

[rtibblesbot]

Correct fix for the P0 regression — the two-part approach (restoring created_by_id so the Celery task can authenticate, plus returning unpublishable to the frontend) addresses both failure modes cleanly.

CI: frontend tests, linting, and build passing; Python unit tests still running. No screenshots in PR; serverSync.js is a data-layer file whose Publish-button behavior can't be screenshot-verified without a running server — author's manual verification steps are documented in the PR.

• suggestion: see inline on test_community_library_submission.py
• suggestion: see inline on serverSync.spec.js
• praise: see inline on community_library_submission.py

---

@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]

praise: Clear, self-documenting comment on why unpublishable=True — makes the intent explicit so future readers don't accidentally remove it thinking it's redundant.

[rtibblesbot]

suggestion: The existing test_resolve_submission__accept_correct verifies that a Change with change_type=ADDED_TO_COMMUNITY_LIBRARY is created, but doesn't assert created_by_id or unpublishable=True. Since restoring created_by_id is the root cause of the P0, consider adding assertions like:

change = Change.objects.get(
    channel=self.submission.channel,
    change_type=ADDED_TO_COMMUNITY_LIBRARY,
)
self.assertEqual(change.created_by_id, self.admin_user.id)
self.assertTrue(change.unpublishable)

This guards against the specific regression being fixed.

[rtibblesbot]

suggestion: The handleMaxRevs logic change (filtering !c.unpublishable) isn't covered by any test in serverSync.spec.js. The handleMaxRevs function as a whole has no unit test. A case like:

// response contains only an unpublishable change → unpublished_changes should NOT be set

...would lock in the fix and catch any future regression. Not strictly blocking for a P0 hotfix, but worth tracking.

[rtibblesbot]

Both prior suggestions addressed — no new findings.

Prior findings

Resolved:

• Test assertions for created_by_id and unpublishable (suggestion) — test now uses .get() and asserts both fields at lines 727-728
• handleMaxRevs unpublishable filtering test (suggestion) — new test added covering the all-unpublishable case

2/2 prior findings resolved.

---

CI passing (linting). Frontend changes are data-layer only (serverSync.js) with no visual component — behaviour is covered by the new serverSync.spec.js test.

---

@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

[rtibbles]

Code changes are very straight forward. One possible small bit of future cleanup, but also not required now.

[rtibbles]

I mean.. I guess it would make sense for "PUBLISHED" to just be an unpublishable_change!