Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
import { queueChange, debouncedSyncChanges } from '../serverSync';
import { CreatedChange } from '../changes';
import db from '../db';
import { Session, Task } from 'shared/data/resources';
import { Channel, Session, Task } from 'shared/data/resources';
import client from 'shared/client';
import { CHANGES_TABLE, CURRENT_USER, TABLE_NAMES } from 'shared/data/constants';
import { CHANGE_TYPES, CHANGES_TABLE, CURRENT_USER, TABLE_NAMES } from 'shared/data/constants';
import { mockChannelScope, resetMockChannelScope } from 'shared/utils/testing';

async function makeChange(key, server_rev) {
Expand Down Expand Up @@ -225,4 +225,37 @@ describe('ServerSync tests', () => {
expect(dbTask.status).toEqual(task.status);
}
});

it('should not set unpublished_changes when response contains only unpublishable changes', async () => {
const channelId = 'test-channel-unpublishable';
await Channel.table.put({ id: channelId });

client.post.mockResolvedValue({
data: {
disallowed: [],
allowed: [],
returned: [],
errors: [],
successes: [
{
channel_id: channelId,
server_rev: 100,
created_by_id: 'some-user-id',
type: CHANGE_TYPES.UPDATED,
unpublishable: true,
},
],
maxRevs: [],
tasks: [],
},
});

await debouncedSyncChanges();

const channel = await Channel.table.get(channelId);
expect(channel.unpublished_changes).not.toBe(true);

// Manual clean up
await Channel.table.delete(channelId);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,12 @@ function handleMaxRevs(response, userId) {
maxRevs[`${MAX_REV_KEY}.${channelId}`] = channelChanges[0].server_rev;
const lastChannelEditIndex = findLastIndex(
channelChanges,
c => !c.errors && !c.user_id && c.created_by_id && c.type !== CHANGE_TYPES.PUBLISHED,
c =>
!c.errors &&
!c.user_id &&
c.created_by_id &&
c.type !== CHANGE_TYPES.PUBLISHED &&
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

!c.unpublishable,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

);
const lastPublishIndex = findLastIndex(
channelChanges,
Expand Down
1 change: 1 addition & 0 deletions contentcuration/contentcuration/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -3817,6 +3817,7 @@ def serialize(cls, change):
"channel_id": get_attribute(change, ["channel_id"]),
"user_id": get_attribute(change, ["user_id"]),
"created_by_id": get_attribute(change, ["created_by_id"]),
"unpublishable": get_attribute(change, ["unpublishable"]),
}
)
return datum
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -720,12 +720,12 @@ def test_resolve_submission__accept_correct(self, apply_task_mock):
self.assertEqual(resolved_submission.resolved_by, self.admin_user)
self.assertEqual(resolved_submission.date_updated, self.resolved_time)

self.assertTrue(
Change.objects.filter(
channel=self.submission.channel,
change_type=ADDED_TO_COMMUNITY_LIBRARY,
).exists()
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)
apply_task_mock.fetch_or_enqueue.assert_called_once_with(
self.admin_user,
channel_id=self.submission.channel.id,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -320,6 +320,7 @@ def _add_to_community_library(self, submission):
categories=submission.categories,
country_codes=country_codes,
),
created_by_id=submission.resolved_by_id,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

# This change is not publishable and should not trigger publish-related logic
unpublishable=True,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

)
Expand Down
1 change: 1 addition & 0 deletions contentcuration/contentcuration/viewsets/sync/endpoint.py
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,7 @@ def return_changes(self, request, channel_revs):
"table",
"change_type",
"kwargs",
"unpublishable",
)
.order_by("server_rev")[:CHANGE_RETURN_LIMIT]
)
Expand Down
Loading