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

Feature branch/tet 613 refactor investment project #6465

Merged

Conversation

Richard-Pentecost
Copy link
Contributor

@Richard-Pentecost Richard-Pentecost commented Jan 26, 2024

Description of change

Refactor project investment pages so that the layout is rendered before the resource is loaded. Create new layouts for the investment pages to use so that inline resources can be rendered for better user experience if there are errors.

Test instructions

These steps use 'Super Awesome Project' as an example investment project but you can use any investment project to test the changes.

To carry out these steps, run the dev environment locally.

  1. From the Data Hub homepage, navigate to the Investments page

  2. Select an Investment Project to load the Project details page e.g. Super Awesome Project

  3. You should see the page layout load before the page resources i.e. you should be able to see the page header and left hand navigation whilst the Investment information is loaded (see screen shots below for an example). Loading wheels should appear inline.

  4. To test that the page displays error messages inline when an investment cannot be loaded, update the ProjectId to an unrecognisable string
    e.g.

const ProjectTeam = () => {
// const { projectId } = useParams()
const projectId = 'asdf'

  1. Repeat the previous steps for the following pages within the investment project:

Screenshots

Before

Appearance of Project Team page with no errors:
Screenshot 2024-01-31 at 10 08 04

After

Appearance of Project Team page with no errors:
Screenshot 2024-01-31 at 10 09 59

Before

Appearance of Project Team page with errors:
Screenshot 2024-01-31 at 10 52 40

After

Appearance of Project Team page with errors:
Screenshot 2024-01-31 at 10 44 06

Checklist

  • Has the branch been rebased to main?
  • Automated tests (Any of the following when applicable: Unit, Functional or End-to-End)
  • Manual compatibility testing (Browsers: Chrome, Firefox, Edge, Safari)

@Richard-Pentecost Richard-Pentecost requested a review from a team as a code owner January 26, 2024 15:19
Copy link

cypress bot commented Jan 26, 2024

Passing run #50926 ↗︎

0 24 0 0 Flakiness 0

Details:

refactor InvestmentProjectAdmin component to use common InvestmentName component
Project: data-hub-frontend Commit: fee5e75413
Status: Passed Duration: 02:01 💡
Started: Feb 8, 2024 10:58 AM Ended: Feb 8, 2024 11:00 AM

Review all test suite changes for PR #6465 ↗︎

Copy link
Contributor

@oliverjwroberts oliverjwroberts 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 you have made all look good to me and are working as expected on my end. In fact, I really like how the pages now load.

However, I did note that when testing if the error messages load with an unknown project ID, clicking on the Admin tab caused an error and the app to crash.

After clicking on the Evidence tab, the error messages appear as expected:

image

After clicking on the Admin tab, the app crashed:

image

Console output:

error: uncaughtException: Cannot destructure property 'id' of 'res.locals.investment' as it is undefined.
TypeError: Cannot destructure property 'id' of 'res.locals.investment' as it is undefined.
[nodemon] app crashed - waiting for file changes before starting...

This can also be replicated on the staging environment with a 504 error, so most likely a pre-existing issue.

I'd be happy to discuss further, but thought we might want to investigate some more before publishing this change as it could potentially increase the chance of users accidentally experiencing the issue.

@Richard-Pentecost Richard-Pentecost force-pushed the feature-branch/TET-613-RefactorInvestmentProject branch from e483fa2 to 863c26b Compare February 8, 2024 10:41
Richard Pentecost and others added 23 commits February 8, 2024 10:42
…ce, create new layout and layout header for investments
@Richard-Pentecost Richard-Pentecost force-pushed the feature-branch/TET-613-RefactorInvestmentProject branch from 863c26b to c1258fb Compare February 8, 2024 10:42
@Richard-Pentecost Richard-Pentecost force-pushed the feature-branch/TET-613-RefactorInvestmentProject branch from e0cae66 to fee5e75 Compare February 8, 2024 10:48
@Richard-Pentecost Richard-Pentecost merged commit 5696a8d into main Feb 8, 2024
16 checks passed
@Richard-Pentecost Richard-Pentecost deleted the feature-branch/TET-613-RefactorInvestmentProject branch February 8, 2024 16:02
chopkinsmade pushed a commit that referenced this pull request Feb 8, 2024
…-RefactorInvestmentProject

generated from commit 5696a8d
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.

4 participants