Skip to content

Pinned Images#1519

Open
annehaley wants to merge 8 commits into
masterfrom
image-pin-sort
Open

Pinned Images#1519
annehaley wants to merge 8 commits into
masterfrom
image-pin-sort

Conversation

@annehaley

Copy link
Copy Markdown
Contributor

This PR does the following:

  1. Add the pinned boolean field to the Image model
  2. Add an endpoint to set the value of the pinned field on an Image. Only staff users are permitted to set the pinned state.
  3. Add an actions menu with a pin/unpin button to the Image Detail page (created to mimic the actions menu on the Collection Detail page). The pin/unpin button is only visible to staff users. Because the pin/unpin button is the only thing in this menu so far, the whole menu is only visible to staff users. We can remove that extra conditional if other non-staff options are added to the menu.
  4. Change the default sorting of the Image browser page to ("-pinned", "created") so that pinned Images are shown first.
  5. Add a new custom pagination class DynamicOrderCursorPagination, which extends CursorPagination. Using this pagination class allows the user to specify the ordering with the order_by query parameter. The image_list and image_search endpoints now use this pagination class. This will allow us to prioritize pinned images in the gallery by specifying ?order_by=-pinned,created.
  6. Fix a bug in the CursorPagination class, where only the first field in the ordering was honored during pagination.
  7. Add tests for the above behavior (ensure only staff can set pinned state, ensure list and search endpoints can be ordered by pinned state, ensure menu behavior with Playwright)

Comment thread isic/core/templates/core/image_detail/actions_menu.html Outdated
function imageActions() {
return {
errorMessage: '',
setPinned(pinned) {

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.

This should just be an async function, using await and try...catch instead of .then and .catch.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Rebased on #1527, addressing that refactoring here and in all other places that use .then

window.location.reload();
}).catch(function(error) {
if (error.response && error.response.data && error.response.data.error) {
alert(error.response.data.error);

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'm not sure if we need to be so aggressive about error catching. We have Sentry, so we can just get an immediate report if something fails in prod.

Even if we do want to keep the user-facing error message, we should re-throw the exception, so it ultimately bubbles up uncaught and Sentry picks it up.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Rebased on #1527; error handling is more consistent now. We raise a generic error and rethrow the exception so Sentry can pick it up.

Comment thread isic/core/api/image.py
response={200: None, 400: dict, 403: dict},
include_in_schema=False,
)
def image_set_pinned(request, id: int, payload: SetPinned):

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.

Should this just be a form endpoint instead? That's a real question, not a suggestion.

@danlamanna Thoughts?

@annehaley

Copy link
Copy Markdown
Contributor Author

@brianhelba Thanks for the suggestions. I can definitely make these changes, but I'll first ask if we should make the same changes in the following places:

I had copied code from each of these places so I could mimic the pinning behavior of collections. Should we try to keep them the same?

@brianhelba

Copy link
Copy Markdown
Member

Good point. Since this is already getting to be a larger PR, I'd say is actually fine to keep here as-is, then make a follow-up PR to improve all these places.

@annehaley annehaley changed the base branch from master to async-and-errs June 11, 2026 17:55
@annehaley annehaley changed the base branch from async-and-errs to master June 11, 2026 18:51
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.

2 participants