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

feat: add has_send_component to flows table #4113

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

RODO94
Copy link
Contributor

@RODO94 RODO94 commented Jan 7, 2025

What does this PR do?

To support the work on organising flows, we want to try understand which services are Submission and which are Guidance. One tactic for this is to look for a Send component in the flow data. This would say that this services submits data rather than providing guidance.

Linked to this Trello tick: https://trello.com/c/7av8lsVr/3160-epic-as-an-editor-i-want-to-have-a-way-to-organise-my-flows-so-that-i-can-easily-find-what-im-looking-for

Copy link

github-actions bot commented Jan 7, 2025

Pizza

Deployed 1786a7f to https://4113.planx.pizza.

Useful links:

@@ -0,0 +1,2 @@
alter table "public"."published_flows" add column "has_send_component" boolean
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the default false hopefully ensures it's false always and I don't need to populate everything

Copy link
Member

@jessicamcinchak jessicamcinchak Jan 9, 2025

Choose a reason for hiding this comment

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

nit: I don't mean this to sound harsh, but I find comments like this quite confusing to review - are you confident that default 'false' works as you expect it to (based on your local env and how pizza built) or are you asking an implementation question that you'd like a second opinion on?

"Hopefully ensures" sounds like you're not confident in what you've done here and also have not double-checked the pizza's published_flows table to confirm, which is not the right attitude for database migrations !!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nah not harsh at all! I was planning to come back with confirmation that it was all good, apologies for the nature of the changes / issues on this one. Thanks for giving feedback even when you think it's harsh, always really appreciate it

Copy link

github-actions bot commented Jan 8, 2025

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

Updated Tables (1)

  • public.published_flows permissions:

    insert select update delete
    api / /
    demoUser / /
    platformAdmin / /
    public /
    teamEditor / /
    9 added column permissions
    insert select update
    api ➕ has_send_component ➕ has_send_component
    demoUser ➕ has_send_component
    platformAdmin ➕ has_send_component
    public
    teamEditor ➕ has_send_component

@RODO94 RODO94 requested a review from jessicamcinchak January 8, 2025 10:09
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.

The pizza here has an empty database that has failed to sync - because this is a database migration, I would expect to please be able to check the Hasura console for new values !

AS $function$
WITH current_published_flow as (
SELECT
id, data, created_at, flow_id, publisher_id, summary, has_send_component
Copy link
Member

Choose a reason for hiding this comment

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

👍

@@ -0,0 +1,2 @@
alter table "public"."published_flows" add column "has_send_component" boolean
Copy link
Member

@jessicamcinchak jessicamcinchak Jan 9, 2025

Choose a reason for hiding this comment

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

nit: I don't mean this to sound harsh, but I find comments like this quite confusing to review - are you confident that default 'false' works as you expect it to (based on your local env and how pizza built) or are you asking an implementation question that you'd like a second opinion on?

"Hopefully ensures" sounds like you're not confident in what you've done here and also have not double-checked the pizza's published_flows table to confirm, which is not the right attitude for database migrations !!

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