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

design-docs: introduce rolling stock collections #197

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

multun
Copy link
Contributor

@multun multun commented May 13, 2024

No description provided.

@multun multun force-pushed the rolling-stock-collections branch from e3a1bef to 67f3088 Compare May 13, 2024 16:07
@multun multun requested a review from leovalais May 13, 2024 16:07
Copy link
Contributor

@leovalais leovalais left a comment

Choose a reason for hiding this comment

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

LGTM as is, maybe a few endpoints could be useful as implementation guidelines.

We could introduce the possibility of locking a collection upfront as this feature is likely to be requested down the line I believe.

@multun
Copy link
Contributor Author

multun commented May 13, 2024

There will be no need for locking once permissions are implemented: an infra is locked if nobody has writer or greater rights

@leovalais
Copy link
Contributor

There will be no need for locking once permissions are implemented: an infra is locked if nobody has writer or greater rights

Ofc, you're right.

@multun multun force-pushed the rolling-stock-collections branch from 67f3088 to d572d62 Compare May 13, 2024 21:58
@multun
Copy link
Contributor Author

multun commented May 13, 2024

I've added a sample migration sql. I trust the implementer with coming up with the crud for this.


## Context and requirements

As of the writting of this proposal, rolling stocks:
Copy link
Contributor

@SergeCroise SergeCroise Jul 30, 2024

Choose a reason for hiding this comment

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

Suggested change
As of the writting of this proposal, rolling stocks:
As of the writing of this proposal, rolling stocks:

https://www.merriam-webster.com/dictionary/writing

Copy link
Contributor

@SergeCroise SergeCroise left a comment

Choose a reason for hiding this comment

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

@multun , please review my suggestion

@multun
Copy link
Contributor Author

multun commented Aug 30, 2024

@SergeCroise The suggestion is good but I can't apply it without closing this PR: I don't have push access to the repository anymore. I see two possible options:

  • push to the branch yourself, and resolve your own comment
  • I could fork, close this PR and open a new one

## Proposal

A concept of rolling stock collection would be introduced:
- each rolling stock must belong to exactly one collection
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this constraint, that means that we will need to duplicate a rolling stock for each collection we want it to be in.

- when creating a scenario, rolling stock collections need to be added

Rolling stock collections have:
- a name
Copy link
Contributor

Choose a reason for hiding this comment

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

And probably an identifier? (but based on the SQL below, I guess the answer to my answer is yes)

```sql
create table rolling_stock_collection(
id bigserial generated always as identity primary key,
name text not null,
Copy link
Contributor

Choose a reason for hiding this comment

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

  description text not null,

create index on rolling_stock_collection(created_by);
create index on rolling_stock(collection);
```

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we also would like to add a rolling_stock_collection_id to the timetable table, isn't it?

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