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

refactor: Drop redundant columns from teams table #3636

Closed
wants to merge 4 commits into from

Conversation

RODO94
Copy link
Contributor

@RODO94 RODO94 commented Sep 6, 2024

What does this PR do?

I have dropped all columns in the teams table that have been moved to the team_settings table. I have updated the necessary scripts in seed-database/write.

Ran GH regression tests and all passed

Copy link

github-actions bot commented Sep 6, 2024

🤖 Hasura Change Summary compared a subset of table metadata including permissions:

Updated Tables (1)

  • public.teams permissions:

    insert select update delete
    api /
    platformAdmin / / /
    public /
    teamEditor /
    23 removed column permissions
    insert select update
    api ➖ boundary
    ➖ notify_personalisation
    ➖ reference_code
    ➖ settings
    ➖ submission_email
    platformAdmin ➖ boundary
    ➖ notify_personalisation
    ➖ settings
    ➖ submission_email
    ➖ boundary_bbox
    ➖ notify_personalisation
    ➖ reference_code
    ➖ settings
    ➖ notify_personalisation
    ➖ settings
    ➖ submission_email
    public ➖ boundary
    ➖ notify_personalisation
    ➖ reference_code
    ➖ settings
    teamEditor ➖ notify_personalisation
    ➖ reference_code
    ➖ settings

Untracked Tables (1)

  • public.teams_summary

Copy link

github-actions bot commented Sep 6, 2024

Pizza

Deployed 93df80f to https://3636.planx.pizza.

Useful links:

@RODO94 RODO94 requested a review from a team September 6, 2024 10:07
@RODO94 RODO94 marked this pull request as ready for review September 6, 2024 10:07
@RODO94
Copy link
Contributor Author

RODO94 commented Sep 6, 2024

Copy link
Member

@jessicamcinchak jessicamcinchak left a comment

Choose a reason for hiding this comment

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

Two adjustments please!

@RODO94 RODO94 requested a review from a team September 9, 2024 09:49
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.

Please also take a look at the migration files at planx-new/hasura.planx.uk/migrations/1692010483148_add_team_boundary - there's an orphaned boundary_bbox() function that should also be tidied up here now we're handling this on the frontend.

One thing to consider here - just updating db columns won't update the types directly. We'll also need a corresponding change in planx-core which removes these properties from the Team type (if they're present), and then import this type into the wider codebase to pick up any cascading changes. There is a chance that all tests are passing because types and mocks are unaware of the changes made in this PR.

- slug
- submission_email
- updated_at
computed_fields:
- boundary_bbox
Copy link
Contributor

@DafyddLlyr DafyddLlyr Sep 9, 2024

Choose a reason for hiding this comment

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

I think the computed field boundary_bbox is also redundant now right?

Comment on lines -2136 to -2137
- table:
name: teams_summary
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure where my previous comment went but this is still being incorrectly removed here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry! That was in the submission email PR, but this is the exact same issue. Removing columns and not repointing the View creation. My bad for missing this again

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@DafyddLlyr for this one, if we merge my submission_email PR (#3640), I have already changed the references in the View creation, so this won't be an issue?

Could be a nice candidate to re-do this on a clean branch once #3640 is merged and reopen a new PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Otherwise, I would be repeating code across the migrations

@RODO94 RODO94 closed this Sep 10, 2024
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.

3 participants