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

chore: refactor formatRawProjectTypes (take 2) #3421

Merged
merged 6 commits into from
Jul 15, 2024

Conversation

jessicamcinchak
Copy link
Member

@jessicamcinchak jessicamcinchak commented Jul 12, 2024

See theopensystemslab/planx-core#445 for context!

Regression tests timing out on this branch - no errors, not sure why yet ! https://github.com/theopensystemslab/planx-new/actions/runs/9911311940 (likely related to team_settings - let's merge to main & fix forward!)

Will drop project_types table in a followup PR.

Copy link

github-actions bot commented Jul 12, 2024

Removed vultr server and associated DNS entries

@jessicamcinchak jessicamcinchak marked this pull request as ready for review July 12, 2024 16:47
@jessicamcinchak jessicamcinchak requested a review from a team July 12, 2024 16:47
@jessicamcinchak jessicamcinchak marked this pull request as draft July 12, 2024 17:52
@jessicamcinchak jessicamcinchak marked this pull request as ready for review July 15, 2024 07:59
Copy link
Contributor

@DafyddLlyr DafyddLlyr left a comment

Choose a reason for hiding this comment

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

One suggested simplification / tidy up 👍

Comment on lines 177 to 178
const fetchProjectType = () => {
const projectType = formatRawProjectTypes(rawProjectTypes);
Copy link
Contributor

@DafyddLlyr DafyddLlyr Jul 15, 2024

Choose a reason for hiding this comment

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

nit: We have to use this slightly awkward syntax when calling an async function inside a useEffect() which required us to define the async function in scope.

Now it's no longer async I think we can just simplify this by dropping the useEffect() and associated useState() - this isn't a reactive value, just a formatted prop.

const projectType = formatRawProjectTypes(rawProjectTypes);

// or if the above is a little slow/intensive (I don't really think so?)

const projectType = useMemo(formatRawProjectTypes(rawProjectTypes), [rawProjectTypes])

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch ! Updated here 24ba79f (no indication it needs a useMemo, but we can keep an eye on that in prod)

@DafyddLlyr DafyddLlyr force-pushed the jess/refactor-formatRawProjectTypes-take-2 branch from a0cfa19 to c23ea89 Compare July 15, 2024 12:16
@DafyddLlyr DafyddLlyr force-pushed the jess/refactor-formatRawProjectTypes-take-2 branch 3 times, most recently from bfaaf8b to 4e14dbf Compare July 15, 2024 13:48
- Bump planx-core
- Add missing team_settings queries
- Add reference_code for BOPS e2e tests
@DafyddLlyr DafyddLlyr force-pushed the jess/refactor-formatRawProjectTypes-take-2 branch from 4e14dbf to 02ed541 Compare July 15, 2024 14:09
@DafyddLlyr
Copy link
Contributor

DafyddLlyr commented Jul 15, 2024

@jessicamcinchak Finally resolved the outstanding issues here -

  • A few team setting queries and types I missed (sorry!)
  • The very frustrating TypeError: Cannot read properties of undefined (reading 'alteredNodes') we've hit a few times with Playwright tests (basically leads to timeouts and unhelpful reports as they don't catch the dialog this pops up in).
    • As with last time, this was resolved by updating Axios
  • A project type update in E2E regressions tests

✅ Regression tests passing against branch here - https://github.com/theopensystemslab/planx-new/actions/runs/9944116858/job/27469441913

image

@DafyddLlyr DafyddLlyr merged commit 84670ae into main Jul 15, 2024
17 checks passed
@DafyddLlyr DafyddLlyr deleted the jess/refactor-formatRawProjectTypes-take-2 branch July 15, 2024 18:00
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