Handle None in course breadcrumbs

[mbasaglia]

Description

Under some circumstances sequence_title might be None, which causes the breadcrumbs to display "None".

This ensures that doesn't happen and avoids stray breadcrumb separators.

See https://github.com/edx/edx-platform/blob/13a70fcaa64428fd564e912aa370a96ce9ee3316/lms/djangoapps/courseware/views/index.py#L518-L531

The issue is visible for a Learner user viewing a timed exam:

Since the template assumed there is always a final breadcrumb but this might no longer be the case, I've added conditions to hide the separators between crumbs when they aren't needed.

Testing instructions

• Enable Special exam types / timed exams
• Create a timed exam
• Add a unit in such exam
• Log in as a Learner (the issue is not visible when previewing, so you need to be logged in as a Learner)
• Access said unit

[openedx-webhooks]

Thanks for the pull request, @mbasaglia! I've created OSPR-5850 to keep track of it in JIRA, where we prioritize reviews. Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket:

• supporting documentation
• Open edX discussion forum threads
• timeline information ("this must be merged by XX date", and why that is)
• partner information ("this is a course on edx.org")
• any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

[natabene]

@mbasaglia Thank you for your contribution. Is this ready for our review?

[mbasaglia]

@mbasaglia Thank you for your contribution. Is this ready for our review?

Yeah, it's just a simple template change

[arch-bom-gocd-alerts]

📣 💥 Heads-up: You must either rebase onto master or merge master into your branch to avoid breaking the build.

We recently removed diff-quality and introduced lint-amnesty. This means that the automated quality check that has run on your branch doesn't work the same way it will on master. If you have introduced any quality failures, they might pass on the PR but then break the build on master.

This branch has been detected to not have commit 2e33565 as an ancestor. Here's how to see for yourself:

git merge-base --is-ancestor 2e335653 mattia/BB-4305 && echo "You're all set" || echo "Please rebase onto master or merge master to your branch"

If you have any questions, please reach out to the Architecture team (either #edx-shared-architecture on Open edX Slack or #architecture on edX internal).

[edx-status-bot]

Your PR has finished running tests. There were no failures.

[Agrendalath]

@mbasaglia, although it might be tempting to skip the internal review for small changes, we should never do this, especially for upstream PRs. Please read What to include in a pull request and How to open a pull request section of our handbook and apply things mentioned there (e.g. the Jira ticket is missing in the PR's title). Additionally, please apply the following changes:

• Remove the code preview (edx-platform/lms/djangoapps/courseware/views/index.py) from the PR's description. You can add a link to this code, but the preview makes the description much less readable, especially that it's not strictly related to these changes.
• Change the "Under some circumstances" in the commit's body to a specific example. A person reading the message will have much more context thanks to this. We should explain that this is happening when the sequence title cannot be displayed to the user, which happens, e.g. before they start a timed exam.

@natabene, this is not ready for your review yet. Sorry for the confusion.

[Agrendalath]

@mbasaglia, what's the point of having these checks? Why don't we just move these arrows to the next items, like I've suggested to you before?

We'll also need to move the fa-angle-right arrows to be displayed before the text to avoid having a trailing arrow when the unit's name is unavailable.

Instead of duplicating conditions, we could simply do something like:

% if sequence_title:
    <span class="icon fa fa-angle-right" aria-hidden="true"></span>
    <span class="nav-item nav-item-sequence">${sequence_title}</span>
% endif</span>

[mbasaglia]

it's to also avoid extra leading icons, since you can't assume you have a previous item

[Agrendalath]

@mbasaglia, in this case, we can completely remove these spans and rely on the CSS instead of adding extra logic to templates.
We can remove this and add the following as the last rule of the .nav-item class:

&:not(:last-child)::after {
    content: '\f105';
    font-family: FontAwesome;

    @include margin-left($baseline/4);

    display: inline-block;
    color: $body-color;

    @include rtl {
      @include transform(rotateY(180deg));
    }
}

Note: I've intentionally skipped media-breakpoint-down here, as it's broken on mobile resolutions.

[Agrendalath]

Note for OpenCraft - the related ticket is BB-4305.

[sarina]

Closing per Piotr's request - OC will re-open a PR for this work.

[openedx-webhooks]

@mbasaglia Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future.