Add substitute operating period

[Perholehto]

• New table service_calendar.substitute_operating_period to group substitute_operating_days
• New view service_calendar.substitute_operating_period_with_date_range to get periods with date range and linetypes
• New function service_calendar.save_operating_periods handles saving/updating substitute_operating_days for substitute_operating_periods

---

This change is 

[jarkkoka]

Reviewable status: 0 of 15 files reviewed, 5 unresolved discussions (waiting on @Perholehto)

---

migrations/hsl/timetables/1683624912748_create_substitute_operating_period/up.sql line 4 at r1 (raw file):

    id uuid DEFAULT gen_random_uuid(),
    period_name text UNIQUE NOT NULL,
    is_preset BOOLEAN NOT NULL DEFAULT FALSE

Put data types lowercase as above in the previous two cases.

---

migrations/hsl/timetables/1683624912748_create_substitute_operating_period/up.sql line 12 at r1 (raw file):

    CONSTRAINT substitute_operating_period_pkey PRIMARY KEY (id);

COMMENT ON COLUMN service_calendar.substitute_operating_period.period_name IS 'Substitute operating periods name';

period's

---

migrations/hsl/timetables/1683624912748_create_substitute_operating_period/up.sql line 34 at r1 (raw file):

    SELECT
        p.id,
        period_name,

For style consistency, add p. prefix because all the other columns are equipped with table alias prefix.

---

migrations/hsl/timetables/1683624912748_create_substitute_operating_period/up.sql line 50 at r1 (raw file):

CREATE
OR REPLACE FUNCTION service_calendar.save_operating_periods(periods jsonb, removed jsonb) RETURNS SETOF service_calendar.substitute_operating_period LANGUAGE plpgsql VOLATILE AS $$ BEGIN

What are these parameters? They should be documented e.g. what is the expected contents.

---

migrations/hsl/timetables/1683624912748_create_substitute_operating_period/up.sql line 64 at r1 (raw file):

-- Remove operating periods that have been removed
-- Remove each day of the period

You should indent all statements inside the SQL function since it visually looks like this DELETE statement is not part of the function.

[HenrikHartiala]

Reviewed 15 of 15 files at r1, all commit messages.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @Perholehto)

---

-- commits line 2 at r1:
You had nice bullet points to sum the commit in the PR description, you could add them also in the commit body here 👍

Also: is there a ticket about this hasura side? If there is, then you could/should add the "Resolves HSLdevcom/jore4#foobar"

---

migrations/hsl/timetables/1683624912748_create_substitute_operating_period/down.sql line 8 at r1 (raw file):

    ONLY service_calendar.substitute_operating_day_by_line_type DROP COLUMN substitute_operating_period_id;

DROP TABLE service_calendar.substitute_operating_period;

No newline at the end of file.

---

migrations/hsl/timetables/1683624912748_create_substitute_operating_period/up.sql line 2 at r1 (raw file):

CREATE TABLE service_calendar.substitute_operating_period (
    id uuid DEFAULT gen_random_uuid(),

I think this id should be named substitute_operating_period_id to be consistent with other table id naming. If I remember the initial reason why we name these like that and not just "id" was so that we can use for example join -clauses with the USING -clause.

JOIN vehicle_service.vehicle_service vs USING (vehicle_schedule_frame_id)

So this way if the id field matches the other table's foreign key field name, this is possible.

---

migrations/hsl/timetables/1683624912748_create_substitute_operating_period/up.sql line 42 at r1 (raw file):

        MAX(d.end_time) AS end_time
    FROM
        service_calendar.substitute_operating_period p

NIT, I think the table names have been shortened by using all the first letters of the table.

This also will help the readability in the long run, if the same tables are always shortened the same way.

Suggestion:

        service_calendar.substitute_operating_period sop

---

migrations/hsl/timetables/1683624912748_create_substitute_operating_period/up.sql line 43 at r1 (raw file):

    FROM
        service_calendar.substitute_operating_period p
        INNER JOIN service_calendar.substitute_operating_day_by_line_type d ON p.id = d.substitute_operating_period_id

As mentioned in an earlier comment, if the id is named substitute_operating_period_id, you can simplify this clause:
Also here the same comment about the table name shorten. d kind of doesn't ring any bells for me, but if this was sodblt, I would immediately recognise this 👍

Suggestion:

        INNER JOIN service_calendar.substitute_operating_day_by_line_type sodblt USING(substitute_operating_period_id)

---

migrations/hsl/timetables/1683624912748_create_substitute_operating_period/up.sql line 115 at r1 (raw file):

WHERE
    EXISTS(
        SELECT

Im definitely not the best to judge the intendations and linebreaks on SLQ files, but this looks a bit different style than used in other SQL files. So this suggestion might be wrong, but in other files I think the linebreaks and intendations are closer to this?

Suggestion:

DELETE FROM service_calendar.substitute_operating_day_by_line_type sodblt
WHERE EXISTS(
  SELECT 1
  FROM operating_periods
  WHERE sodblt.substitute_operating_period_id = operating_periods.id
    );

---

migrations/hsl/timetables/1683624912748_create_substitute_operating_period/up.sql line 178 at r1 (raw file):

END;

$$;

No newline at the end of file.

[HenrikHartiala]

Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @Perholehto)

---

migrations/hsl/timetables/1683624912748_create_substitute_operating_period/up.sql line 63 at r1 (raw file):

    ) ON COMMIT DROP;

-- Remove operating periods that have been removed

NIT, maybe there is another way of saying this

Suggestion:

-- Remove given (removed jsonb) operating periods.

[HenrikHartiala]

Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @Perholehto)

---

migrations/hsl/timetables/1683624912748_create_substitute_operating_period/up.sql line 2 at r1 (raw file):

Previously, HenrikHartiala wrote…

I think this id should be named substitute_operating_period_id to be consistent with other table id naming. If I remember the initial reason why we name these like that and not just "id" was so that we can use for example join -clauses with the USING -clause.

JOIN vehicle_service.vehicle_service vs USING (vehicle_schedule_frame_id)

So this way if the id field matches the other table's foreign key field name, this is possible.

This also might help readability in UI side, but the downside is that the keyField for apollo needs to be manually added (but this is on the UI side)

[jarkkoka]

Reviewed all commit messages.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @HenrikHartiala and @Perholehto)

---

migrations/hsl/timetables/1683624912748_create_substitute_operating_period/up.sql line 2 at r1 (raw file):

Previously, HenrikHartiala wrote…

This also might help readability in UI side, but the downside is that the keyField for apollo needs to be manually added (but this is on the UI side)

That's right. We have an established naming convention for (primary key) ID columns where the name of the table is included in the column name. We should stick to these conventions because otherwise code becomes messy.

[Perholehto]

Reviewable status: 4 of 15 files reviewed, 12 unresolved discussions (waiting on @HenrikHartiala and @jarkkoka)

---

-- commits line 2 at r1:

Previously, HenrikHartiala wrote…

You had nice bullet points to sum the commit in the PR description, you could add them also in the commit body here 👍

Also: is there a ticket about this hasura side? If there is, then you could/should add the "Resolves HSLdevcom/jore4#foobar"

Added description, but there is no ticket for Hasura, will link this PR to ticket

---

migrations/hsl/timetables/1683624912748_create_substitute_operating_period/down.sql line 8 at r1 (raw file):

Previously, HenrikHartiala wrote…

No newline at the end of file.

Newline added

---

migrations/hsl/timetables/1683624912748_create_substitute_operating_period/up.sql line 2 at r1 (raw file):

Previously, jarkkoka (Jarkko Kaura) wrote…

That's right. We have an established naming convention for (primary key) ID columns where the name of the table is included in the column name. We should stick to these conventions because otherwise code becomes messy.

Id changed it to be in line with other ids. I understand that it's a convention but I slightly disagree with this.

---

migrations/hsl/timetables/1683624912748_create_substitute_operating_period/up.sql line 4 at r1 (raw file):

Previously, jarkkoka (Jarkko Kaura) wrote…

Put data types lowercase as above in the previous two cases.

Changed data types to lowercase

---

migrations/hsl/timetables/1683624912748_create_substitute_operating_period/up.sql line 12 at r1 (raw file):

Previously, jarkkoka (Jarkko Kaura) wrote…

period's

Done.

---

migrations/hsl/timetables/1683624912748_create_substitute_operating_period/up.sql line 34 at r1 (raw file):

Previously, jarkkoka (Jarkko Kaura) wrote…

For style consistency, add p. prefix because all the other columns are equipped with table alias prefix.

Function removed

---

migrations/hsl/timetables/1683624912748_create_substitute_operating_period/up.sql line 42 at r1 (raw file):

Previously, HenrikHartiala wrote…

NIT, I think the table names have been shortened by using all the first letters of the table.

This also will help the readability in the long run, if the same tables are always shortened the same way.

Function removed

---

migrations/hsl/timetables/1683624912748_create_substitute_operating_period/up.sql line 43 at r1 (raw file):

Previously, HenrikHartiala wrote…

As mentioned in an earlier comment, if the id is named substitute_operating_period_id, you can simplify this clause:
Also here the same comment about the table name shorten. d kind of doesn't ring any bells for me, but if this was sodblt, I would immediately recognise this 👍

Function removed

---

migrations/hsl/timetables/1683624912748_create_substitute_operating_period/up.sql line 50 at r1 (raw file):

Previously, jarkkoka (Jarkko Kaura) wrote…

What are these parameters? They should be documented e.g. what is the expected contents.

Function removed

---

migrations/hsl/timetables/1683624912748_create_substitute_operating_period/up.sql line 63 at r1 (raw file):

Previously, HenrikHartiala wrote…

NIT, maybe there is another way of saying this

Function removed

---

migrations/hsl/timetables/1683624912748_create_substitute_operating_period/up.sql line 64 at r1 (raw file):

Previously, jarkkoka (Jarkko Kaura) wrote…

You should indent all statements inside the SQL function since it visually looks like this DELETE statement is not part of the function.

Function removed

---

migrations/hsl/timetables/1683624912748_create_substitute_operating_period/up.sql line 115 at r1 (raw file):

Previously, HenrikHartiala wrote…

Im definitely not the best to judge the intendations and linebreaks on SLQ files, but this looks a bit different style than used in other SQL files. So this suggestion might be wrong, but in other files I think the linebreaks and intendations are closer to this?

Function removed

---

migrations/hsl/timetables/1683624912748_create_substitute_operating_period/up.sql line 178 at r1 (raw file):

Previously, HenrikHartiala wrote…

No newline at the end of file.

Added

[HenrikHartiala]

Reviewed 10 of 11 files at r2, 3 of 3 files at r3, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @jarkkoka and @Perholehto)

---

test/hasura/hsl/timetablesdb/datasets/types.ts line 36 at r2 (raw file):

];
export const SubstituteOperatingPeriodProps: Property[] = [
  'id,',

This seems to be still 'id'

Suggestion:

  'substitute_operating_period_id,',

---

test/hasura/hsl/timetablesdb/datasets/defaultSetup/substitute_operating_period.ts line 7 at r3 (raw file):

    substitute_operating_period_id: '0967a31a-8304-4440-9a8e-18bb67b28166',
    is_preset: false,
    period_name: 'Default korvausjakso',

NIT, english & finnish mixed. Also what does the "default" mean here?

[Perholehto]

Reviewable status: 9 of 18 files reviewed, 7 unresolved discussions (waiting on @HenrikHartiala and @jarkkoka)

---

test/hasura/hsl/timetablesdb/datasets/types.ts line 36 at r2 (raw file):

Previously, HenrikHartiala wrote…

This seems to be still 'id'

Fixed

---

test/hasura/hsl/timetablesdb/datasets/defaultSetup/substitute_operating_period.ts line 7 at r3 (raw file):

Previously, HenrikHartiala wrote…

NIT, english & finnish mixed. Also what does the "default" mean here?

Changed this one period name and others to have more describing naming

[HenrikHartiala]

Reviewed 9 of 9 files at r4, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @jarkkoka)

---

test/hasura/hsl/timetablesdb/datasets/defaultSetup/substitute_operating_period.ts line 7 at r3 (raw file):

Previously, Perholehto (Jarno) wrote…

Changed this one period name and others to have more describing naming

Thanks 👍

[jarkkoka]

Reviewed all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Perholehto)

---

migrations/hsl/timetables/1683624912748_create_substitute_operating_period/up.sql line 2 at r1 (raw file):

Previously, Perholehto (Jarno) wrote…

Id changed it to be in line with other ids. I understand that it's a convention but I slightly disagree with this.

It would be quite a mess if we mixed different conventions. When we have established a convention it is better to stick with it. IMO that is a (much) stronger argument than personal preferences with regard to naming columns.

However, what is still missing here is COMMENT ON TABLE that describes the purpose of the new table introduced here.

[Perholehto]

Reviewable status: 16 of 18 files reviewed, all discussions resolved (waiting on @HenrikHartiala)

---

migrations/hsl/timetables/1683624912748_create_substitute_operating_period/up.sql line 2 at r1 (raw file):

Previously, jarkkoka (Jarkko Kaura) wrote…

It would be quite a mess if we mixed different conventions. When we have established a convention it is better to stick with it. IMO that is a (much) stronger argument than personal preferences with regard to naming columns.

However, what is still missing here is COMMENT ON TABLE that describes the purpose of the new table introduced here.

Added comment for table

Of topic: I'm not saying that my personal preference should be prioritized over an established convention. Just asking stupid questions and trying to find all the conventions... I will follow this convention as a developer in a team should do. It doesn't mean that I need to fully agree with this convention to use it and in future projects I will still suggest using different naming convention...

[jarkkoka]

Reviewed 2 of 2 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Perholehto)

---

migrations/hsl/timetables/1683624912748_create_substitute_operating_period/up.sql line 18 at r5 (raw file):

ALTER TABLE ONLY service_calendar.substitute_operating_day_by_line_type
ADD CONSTRAINT substitute_operating_day_by_line_type_substitute_operating_period_fkey FOREIGN KEY (substitute_operating_period_id) REFERENCES service_calendar.substitute_operating_period(substitute_operating_period_id) ON DELETE CASCADE;

This identifier will be truncated because it exceeds 63 characters. IMO, you should name it so that it doesn't exceed 63 characters.

[Perholehto]

Reviewable status: 17 of 18 files reviewed, 1 unresolved discussion (waiting on @jarkkoka)

---

migrations/hsl/timetables/1683624912748_create_substitute_operating_period/up.sql line 18 at r5 (raw file):

Previously, jarkkoka (Jarkko Kaura) wrote…

This identifier will be truncated because it exceeds 63 characters. IMO, you should name it so that it doesn't exceed 63 characters.

Good catch!

Renamed constraint and also index from below that exceeded 63 characters.

[jarkkoka]

Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Perholehto)

[HenrikHartiala]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Perholehto)