#5697 - Update the levels displayed in breadcrumbs across the app

[elvisdorkenoo]

Update the levels displayed in breadcrumbs across the app

This request is to update how we handle breadcrumbs so that we show all levels except for the level the user themselves belongs to, the top-most level.
Summary of expected behavior here :

#5697

Code review checklist

• Readable: Concise, well named, follows the style guide, documented if necessary.
• Documented: Configuration and user documentation on cht-docs
• Tested: Unit and/or e2e where appropriate
• Internationalised: All user facing text
• Backwards compatible: Works with existing data and configuration or includes a migration. Any breaking changes documented in the release notes.

License

The software is provided under AGPL-3.0. Contributions to this project are accepted under the same license.

[jkuester]

This is a solid start! I have added some questions/suggestions inline.

[latin-panda]

Nice!
I agree with Josh feedback and added a couple more things.

[latin-panda]

I think you need to fix the failing e2e... it's probably not a flaky one 🤔

[latin-panda]

Good progress!! We're almost there! 🤩 I added some suggestions in line

[elvisdorkenoo]

One question, do we change the target branch of the PR from master to 3.17.x ? @jkuester @latin-panda

[jkuester]

Couple minor suggestions, but otherwise I think this is pretty much ready to go!

[jkuester]

@elvisdorkenoo regarding the target branch for this PR, I think we should leave it as master. Once this per is Squashed and merged into master, we can cherrypick the commit into 3.17.x.

[jkuester]

Also, wanted to bring this up for the sake of completeness (before we send this over to QA). @elvisdorkenoo @latin-panda do you think we need any kind of e2e test for this?

I have gone back and forth on this, but I am kinda leaning towards adding a test. This functionality depends on interacting with multiple services. The current unit tests have properly stubbed out the behavior of those services, but it seems like a good idea to have at least one e2e test that verifies the breadcrumbs are actually displayed as expected in the app when logged in as an offline user....

[elvisdorkenoo]

Also, wanted to bring this up for the sake of completeness (before we send this over to QA). @elvisdorkenoo @latin-panda do you think we need any kind of e2e test for this?

I have gone back and forth on this, but I am kinda leaning towards adding a test. This functionality depends on interacting with multiple services. The current unit tests have properly stubbed out the behavior of those services, but it seems like a good idea to have at least one e2e test that verifies the breadcrumbs are actually displayed as expected in the app when logged in as an offline user....

I tried to take a look to the existing e2e tests to see if there is anything about breadcrumbs as in this PR we are only updating the displayed levels, and for now, there are no e2e for breadcrumbs that I can find.

[latin-panda]

@elvisdorkenoo we have been building the test coverage recently, so I think we still don't have an e2e test for breadcrumbs in specific. As per Gareth comments before, we are supposed to ship PRs with at least 1 e2e test, which I agree :)
Take a look at this file, it's a good sample of how to do an e2e for Reports tab, you just need a selector to check breadcrumbs texts.

Do you think you can complete 1 e2e today? You will have to pay extra attention in how these are written to get less feedback as possible.

[elvisdorkenoo]

@elvisdorkenoo we have been building the test coverage recently, so I think we still don't have an e2e test for breadcrumbs in specific. As per Gareth comments before, we are supposed to ship PRs with at least 1 e2e test, which I agree :) Take a look at this file, it's a good sample of how to do an e2e for Reports tab, you just need a selector to check breadcrumbs texts.

Do you think you can complete 1 e2e today? You will have to pay extra attention in how these are written to get less feedback as possible.

Yes, I will add the e2e test, thanks for the sample.

[latin-panda]

@elvisdorkenoo how is the e2e going? Do you think we review today?

[elvisdorkenoo]

@elvisdorkenoo how is the e2e going? Do you think we review today?

Yes thanks for the help, I did a push with en e2e test for report, you can take a look @latin-panda and @jkuester

[latin-panda]

Nice! I don't have further suggestions regarding logic. However I left some comments regarding format which should be quick to fix, please make sure that CI is fully green after you push the changes.

@jkuester can you please have a look? Thanks for your time.

[latin-panda]

LGTM! Thank you!

[jkuester]

Everything here looks good!

Did we decide we did not need an e2e test for the message's breadcrumbs?

[elvisdorkenoo]

Everything here looks good!

Did we decide we did not need an e2e test for the message's breadcrumbs?

Thank you @jkuester, it's the same for the tasks in #7757 as well. I’ve created this ticket and I will be doing this next as a separate PR