Skip to content
This repository has been archived by the owner on Apr 24, 2024. It is now read-only.

654 updated database documentation #792

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

temmey
Copy link
Contributor

@temmey temmey commented Aug 5, 2023

Basics

  • I added a line to /doc/CHANGELOG.md
  • The PR is rebased with current master.
  • Details of what you changed are in commit messages.
  • References to issues, e.g. close #X, are in the commit messages.
  • The buildserver is happy.

Checklist

  • I have installed and I am using pre-commit hooks
  • I fully described what my PR does in the documentation
    (not in the PR description)
  • I fixed all affected documentation
  • I fixed the introduction tour
  • I wrote migrations in a way that they are compatible with already present data
  • I fixed all affected decisions
  • I added unit tests for my code
  • I added code comments, logging, and assertions as appropriate
  • I translated all strings visible to the user
  • I mentioned every code or binary not directly written or done by me in reuse syntax
  • I created left-over issues for things that are still to be done
  • Code is conforming to our Architecture
  • Code is conforming to our Guidelines
    (exceptions are documented)
  • Code is consistent to our Design Decisions

Review

  • I've tested the code
  • I've read through the whole code
  • Examples are well chosen and understandable

Copy link
Member

@badnames badnames left a comment

Choose a reason for hiding this comment

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

Well done!
As usual just a few minor things.

doc/database/schemata/01er_tables_implemented_diagram.md Outdated Show resolved Hide resolved
doc/database/schemata/01er_tables_implemented_diagram.md Outdated Show resolved Hide resolved
@markus2330
Copy link
Contributor

@temmey let us get this done, please request review when you are ready and/or set labels accordingly

@markus2330
Copy link
Contributor

@temmey let us do this first, it basically only needs that you reread your own text and rebase it.

Copy link
Contributor

@chr-schr chr-schr left a comment

Choose a reason for hiding this comment

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

Some sections are incomplete / not up to date with the database schema.

Comment on lines +18 to +21
INT price
INT generation
TEXT notes
INT owner_id "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.

Fix types

Suggested change
INT price
INT generation
TEXT notes
INT owner_id "NOT NULL"
SMALLINT price
SMALLINT generation
TEXT notes
UUID owner_id "NOT NULL"

Comment on lines +82 to +89
INT zoom_factor "NOT NULL"
INT honors "NOT NULL"
INT visits "NOT NULL"
INT harvested "NOT NULL"
PRIVACY_OPTION privacy "NOT NULL"
TEXT description
GEOGRAPHY location
INT owner_id "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.

Fix types

Suggested change
INT zoom_factor "NOT NULL"
INT honors "NOT NULL"
INT visits "NOT NULL"
INT harvested "NOT NULL"
PRIVACY_OPTION privacy "NOT NULL"
TEXT description
GEOGRAPHY location
INT owner_id "NOT NULL"
SMALLINT zoom_factor "NOT NULL"
SMALLINT honors "NOT NULL"
SMALLINT visits "NOT NULL"
SMALLINT harvested "NOT NULL"
PRIVACY_OPTION privacy "NOT NULL"
TEXT description
GEOGRAPHY location
UUID owner_id "NOT NULL"



plantings {
INT id PK
Copy link
Contributor

Choose a reason for hiding this comment

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

Fix type

Suggested change
INT id PK
UUID id PK

REAL scale_y "NOT NULL"
DATE add_date
DATE remove_date
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Add missing seed_id

Suggested change
}
INT seed_id
}

blossoms {
TEXT title PK
TEXT description
TRACKS track
Copy link
Contributor

Choose a reason for hiding this comment

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

Fix typo

Suggested change
TRACKS track
TRACK track

}


blossoms_gained {
Copy link
Contributor

Choose a reason for hiding this comment

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

Table is called gained_blossoms

Suggested change
blossoms_gained {
gained_blossoms {

Comment on lines +171 to +176
blossoms ||--o{ blossoms_gained : ""





Copy link
Contributor

Choose a reason for hiding this comment

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

Some relationships were missing

Suggested change
blossoms ||--o{ blossoms_gained : ""
blossoms ||--o{ gained_blossoms : ""
gained_blossoms }o--|| users : "gained by"
users ||--o| guided_tours : ""
base_layer_images }o--|| layers : "belongs to"
users ||--o{ seeds : ""
plantings }o--|| seeds : ""

@@ -2,6 +2,8 @@

## `Plant_detail`
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
## `Plant_detail`
## `Plants`

Comment on lines +74 to +90
| **_Column name_** | **_Example_** | **_Description_** |
| :---------------- | :------------ | :--------------------------------------------------------------------------------------------- |
| **id** | 1 |
| **owner_id** | 1 |
| **name** | My Map | only alphanumerical values |
| **is_inactive** | false |
| **last_visit** | 2023-04-04 |
| **honors** | 0 | 0 to infinity |
| **visits** | 0 | 0 to infinity |
| **harvested** | 0 | 0 to infinity, amount of plants harvested on this map |
| **privacy** | protected | privacy_option enum type |
| **description** | Our first map | |
| **creation_date** | 2023-04-04 |
| **deletion_date** | 2023-04-04 |
| **zoom_factor** | 100 | value used in formula "X by X cm", e.g. 100 would mean "100 x 100 cm", range from 10 to 100000 |
| **location** | NULL | PostGis Geodata, location of the map |
| **geometry** | FILLME | Layout of the map. |
Copy link
Contributor

Choose a reason for hiding this comment

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

Not up to date with the database schema

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we shouldn't fully replicate everything in docu but rather only have the essence. Maybe we simply remove/reduce this?

Copy link
Contributor

@markus2330 markus2330 left a comment

Choose a reason for hiding this comment

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

Great job! Maybe a bit reduction and when @chr-schr approves, we can merge.

Comment on lines +74 to +90
| **_Column name_** | **_Example_** | **_Description_** |
| :---------------- | :------------ | :--------------------------------------------------------------------------------------------- |
| **id** | 1 |
| **owner_id** | 1 |
| **name** | My Map | only alphanumerical values |
| **is_inactive** | false |
| **last_visit** | 2023-04-04 |
| **honors** | 0 | 0 to infinity |
| **visits** | 0 | 0 to infinity |
| **harvested** | 0 | 0 to infinity, amount of plants harvested on this map |
| **privacy** | protected | privacy_option enum type |
| **description** | Our first map | |
| **creation_date** | 2023-04-04 |
| **deletion_date** | 2023-04-04 |
| **zoom_factor** | 100 | value used in formula "X by X cm", e.g. 100 would mean "100 x 100 cm", range from 10 to 100000 |
| **location** | NULL | PostGis Geodata, location of the map |
| **geometry** | FILLME | Layout of the map. |
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we shouldn't fully replicate everything in docu but rather only have the essence. Maybe we simply remove/reduce this?

@chr-schr
Copy link
Contributor

chr-schr commented Feb 1, 2024

I would argue to remove the table descriptions. Ideally they would be somehow automatically generated from the rust table definitions such that we only have a single source of truth, but as it stands I anticipate them to always be out of sync.

@markus2330
Copy link
Contributor

If we don't find some minimal descriptions that are still useful to get an overview, we can also get rid of this documentation completely.

What speaks to maybe keep it: It was mostly used to design the relations and might be useful when we do non-trivial extensions.

We can today also speak a bit about how to document GIS systems.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants