Skip to content

Conversation

taoerman
Copy link
Contributor

@taoerman taoerman commented Jun 28, 2025

Summary

Similar to the deployment flow, show a spinner when draft publishing is in progress. When it completes, show the popup.

Spinner screenshot:
Screenshot 2025-06-27 at 22 36 17

Popup screenshot:
Screenshot 2025-06-27 at 22 37 14

References

#5115

Reviewer guidance

Run server locally, make sure celery is on. Upload a staging channel using ricecooker script. Then test "Publish Draft" button.
To test a failure case, update this publish_next function to always throw an error and test in the UI.

@rtibbles rtibbles self-assigned this Jun 28, 2025
@AlexVelezLl AlexVelezLl self-requested a review July 1, 2025 15:07
Copy link
Member

@AlexVelezLl AlexVelezLl left a comment

Choose a reason for hiding this comment

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

Thanks @taoerman! This is looking good! Just left a couple of nitpick comments, and a question for @rtibbles 😄.

channel = publish_channel(
self.request.user.pk,
channel.id,
progress_tracker=progress_tracker,
Copy link
Member

Choose a reason for hiding this comment

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

@rtibbles Just noticed that we are not creating any change event when we set the publishing field to true. This means that this change won't sync between parallel users, is that okay? (I know this is less relevant for the staging tree publishing as it is usually just one chef checking this page at a time 😅, but just checking-in)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For my understanding, "we are not creating any change event when we set the publishing field to true" -> looks like both publish and publish_next are missing this, is that right? Or maybe I'm missing something.

Copy link
Member

Choose a reason for hiding this comment

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

For my understanding, "we are not creating any change event when we set the publishing field to true" -> looks like both publish and publish_next are missing this, is that right? Or maybe I'm missing something.

Yes! Both of them are missing this.

Copy link
Member

Choose a reason for hiding this comment

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

I think the main reason we don't, is that adding a new change event after a publish then means that the channel can then be republished (because it now has changes). In all honesty, I am now sketchy on the details, and I am not confident that the current implementation is handling this correctly - but the fact that there is a pending Published or DraftPublished event should be sufficient for flagging this in the frontend for all users.

As such, I wonder if the correct query to do here should be on the changes table, rather than the channel table, and checking to see if there are any draft published changes that have been applied yet or not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I agree checking the changes table is more direct and cleaner than relying on some field updates on the channel table. Updated.

@rtibbles
Copy link
Member

rtibbles commented Jul 7, 2025

Interested in both your thoughts @taoerman and @AlexVelezLl about using the changes more proactive to monitor for finishing of the publish, rather than relying on changing attributes on the channel itself.

@taoerman
Copy link
Contributor Author

taoerman commented Jul 9, 2025

Interested in both your thoughts @taoerman and @AlexVelezLl about using the changes more proactive to monitor for finishing of the publish, rather than relying on changing attributes on the channel itself.

This sounds great, it's cleaner to check the changes table directly. The current sync will delete a change locally if applied, I updated the live query accordingly. PLTA. Thanks!

@rtibbles
Copy link
Member

Thanks @taoerman, I'll take another look!

Copy link
Member

@rtibbles rtibbles left a comment

Choose a reason for hiding this comment

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

Implementation looks good, manual test seems to work perfectly, thank you @taoerman, this is excellent work, again!

@rtibbles rtibbles merged commit 0a3875b into learningequality:unstable Jul 10, 2025
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants