Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

added unpublish status in site listing #2290

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

umar8hassan
Copy link
Contributor

@umar8hassan umar8hassan commented Sep 2, 2024

What are the relevant tickets?

Fixes https://github.com/mitodl/hq/issues/4858

Description (What does it do?)

This PR aims to add appropriate site status according to its live publish status for the courses in Listing Dashboard.

How can this be tested?

  1. Switch to umar/4858-ocw-studio-course-publication-status-in-course-listing branch in ocw-studio
  2. Create a new course in ocw-studio at http://localhost:8043/sites
  3. Update the necessary details and publish course at live
  4. The status for the course in course listing at http://localhost:8043/sites
  5. The menu for the course should not appear until the course publishing is complete
  6. The status should be pending/not-started initially then to publish/failed.

Additional Testing

  1. To test all the scenarios, change the Publish date, Live publish status, and Unpublish status fields in Website details on Django Admin panel

Screenshots:

Some test scenarios and their respective values in DB
Screenshot 2024-09-03 at 5 43 28 PM
Screenshot 2024-09-03 at 5 43 58 PM

@umar8hassan umar8hassan self-assigned this Sep 3, 2024
@umar8hassan umar8hassan marked this pull request as ready for review September 3, 2024 12:59
@gumaerc gumaerc self-assigned this Sep 3, 2024
Copy link
Contributor

@gumaerc gumaerc left a comment

Choose a reason for hiding this comment

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

The changes to the site listing look good, but it looks like this item from the issue wasn't addressed:

There are no indications in the course publish drawer that the course is unpublished, or is in the process of being unpublished.

What this is trying to say, I think, is that even though my course (test-site-2290) is unpublished here in the site listing:
image

...it is still marked as "succeeded" when you click into the site for editing:
image

Could we also make the status in the upper right corner here (and in the publish drawer) match the status shown on the listing page?

@pdpinch
Copy link
Member

pdpinch commented Sep 6, 2024

Sorry, I'm still thinking about this and I'd like to bring in some UX advice.

To help out the UX team, can you list out all the possible status for:

  1. the site listing page ("Draft," "Published," etc.)
  2. the publish status indicator ("Succeeded","Failed", anything else?)
  3. the publish drawer

@gumaerc
Copy link
Contributor

gumaerc commented Sep 9, 2024

Sorry, I'm still thinking about this and I'd like to bring in some UX advice.

To help out the UX team, can you list out all the possible status for:

1. the site listing page ("Draft," "Published," etc.)

2. the publish status indicator ("Succeeded","Failed", anything else?)

3. the publish drawer
  1. With Umar's change here, the possible values on the listing page (called SitesDashboard) are "Published," "Draft" or "Unpublished." In the site right now outside of this PR, it's only "Published" or "Draft" (sites go back to "Draft" when unpublished currently)
  2. The publish status indicator has the following statuses it can display, based on the build status in Concourse:
const publishStatusIndicatorClass = (status: PublishStatus): string => {
  switch (status) {
    case PublishStatus.NotStarted:
      return "bg-secondary"
    case PublishStatus.Pending:
    case PublishStatus.Started:
      return "bg-warning"
    case PublishStatus.Aborted:
    case PublishStatus.Errored:
      return "bg-danger"
    case PublishStatus.Success:
      return "bg-success"
    default:
      return ""
  }
}
  1. The drawer reuses the PublishStatusIndicator component so it displays the same status above

@mbilalmughal
Copy link

After discussing the issue with Umer, I believe that not having labels for both statuses may still confuse users. For best practices and good UX, it’s important to guide users, and I think labels will help with that. I've proposed a solution to include separate labels for each. Please see the attached image and let me know if this resolves the problem.

In the first screenshot, I proposed the label 'Deployment,' while in the second screenshot, I used 'Site Status.' Umer preferred 'Site Status,' but I would rather avoid using 'Status' twice.

  • Figma Link

  • Reading from the beginning, it would say,
    'Publish Status - Succeeded'
    'Deployment - Failed'

{D1956456-5BEA-418A-BA64-0A6EB8A92B07}

  • 2nd Screenshot
    'Publish Status - Succeeded'
    'Site Status - Failed'

{97D3C1BC-484B-4C76-AC24-6FC7025E33E4}

@pdpinch
Copy link
Member

pdpinch commented Sep 23, 2024

Looks a bit like a Christmas Tree, or a traffic signal. 🙂

I appreciate the desire for clarity, but sometimes more information leads to increased cognitive load and confusion.

  • The values for "publish status" seem transient to me, and not of much use on the list view. They are more valuable in the course publisher drawer, where the user is wondering what's happening right now, and is tempted to press the publish button over and over.
  • Looking at the values in the screenshots, they seem very redundant. Is that just because of the sample you chose?

@mbilalmughal
Copy link

After discussing it with umer & reading the thread
Carey Comment:

I think we should label and show both, but we should see what Peter thinks

"publish status" vs "deployment status"
if not messaged correctly it could cause more confusion though
Consistent labels on both pages

Peter Comment:

They are more valuable in the course publisher drawer

I think on the listing page, we need to either show both statuses for consistency or not show them at all. This means we should either display both or none. Currently, we are only showing one status, which is the deployment status. Also, the use of multiple colors is usually unavoidable when showing different statuses, but I’m open to any suggestions.

Drawer
Drawer-1
Drawer-2
Listing

@pdpinch
Copy link
Member

pdpinch commented Oct 1, 2024

Sorry guys, this still isn't working for me.

First, I think there are two different bits of information to communicate. One is the current state of the course -- is it never published, published or unpublished (to production). Most of the time, this is all the user wants to know. Then there's the state of change of the publication. that could be requested by "not started", "in progress", "failed" or "succeeded". Most of the time this is static.

Second, I think we should be judicious about what information to show. For the list view, the most common use case is to lok through the list and see if a course has been published or not. The state of change is usually irrelevant and only of interest to someone who cares about that specific course. Can we give that less prominence? Maybe put it in a hover state, or a drop-down, or behind some of kind of "updating..." indicator?

I'll discuss this a bit with Jen today to validate my assumptions about values and priority.

@pdpinch
Copy link
Member

pdpinch commented Jan 16, 2025

@umar8hassan can you pick this up again and use it to close https://github.com/mitodl/hq/issues/6017 ?

Let's narrow the scope and remove any changes to the publish drawer. We can save those for a future issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants