bastislack / highline-freestyle

Webapp for Highline Freestyle
GNU General Public License v3.0
10 stars 9 forks source link

Merge predefined* and user* tables #239

Closed JulianDietzel closed 9 months ago

JulianDietzel commented 1 year ago

The user tricks and the predefined tricks, just as the user combos and the predefined combos are stored in separate tables in the database. Yet, it is made sure in almost every function that adds to those tables, that there are no duplicate ids between the tables. Also, when tricks or combos are fetched from the database, the predefined and the user tables are almost always if not always merged. This makes the code harder to read, makes functions longer (as always both tables need to be queried and the results need to be merged) and creates the need for functions like the mergeLists one.

Couldn't the tables just be merged into one, with an additional column isPredefined, which just stores a boolean value and specifies whether the trick is a predefined one or not. I'd estimate that this would shorten the db.js file by about 20% and increase readability a lot. Also it would prevent the tables from getting out of sync, if someone were to accidentally only change / update one of the tables but not the other.

I'd be interested if there is perhaps a good reason for the current architecture that I am missing. If not, I'd suggest merging both the predefinedTricks and userTricks tables and the predefinedCombos and userCombos tables.

bastislack commented 1 year ago

@weberax was talking about a database refactoring. So maybe this is something we can discuss here?

weberax commented 1 year ago

I like the idea!

The original idea of having two different tables, was

  1. to have the predefinedTricks seperate to update them easily, without tricks getting mixed up (and of course keeping the user data)
  2. and always keep the orginal details of a trick, so that when a user changes attributes of this trick, he can reset them, by just deleting the entry in userTricks.

But since we decided to have the ids in the google form fix, 1. should not be a problem anymore. And 2. should also not be a problem.

So to summarize what needs to be done (analogous with the combos):

This might also fix #240

WaldemarLehner commented 1 year ago

Co-Authored by @JulianDietzel

The Document defines changes to the Database Structure that we deem useful. The first section will be an Outline of the proposed changes. The following sections will talk about the details, the reasons behind decisions, considered alternatives, etc.

Outline

predefinedCombos and userCombos get merged into combos . predefinedTricks and userTricks get merged into tricks . versions gets dropped and replaced with a hashing mechanism. A new metadata-Table gets added. It defines user-defined metadata for both predefined and user-defined tricks and combos.

The following text will talk about Tricks, but note that all mentioned paradigms apply to Combos, too.

The user will no longer be able to modify predefined Tricks. They can however use predefined Tricks as a template.

The Primary Keys get turned into composite keys. Instead of indexing an Entry by the id, two values ([id, trickType]) are used for indexing. TrickType is an Enum stored as a string and validated with zod during insertion. This means that there can be multiple tricks with the same ID as we can have one Trick identified by [1, UserTrick] and another by [1, PredefinedTrick]. This way we can eliminate ID collisions between Predefined and User Tricks.

Below you can see the new proposed Database Structure.

db_schema

Notes

  1. This might need a better name. We were also thinking about TrickVariant, but this will get confusing down the line in case "Trick Variations" get added.
  2. This structure only works if Metadata is identical between Combos and Tricks. Can we assume this? Or are there any values that are specific to Combos or Tricks?
  3. This note is regarding a new Trick- and ComboType: Orphan. An "Orphan" defines a predefined trick where the user has added any kind of Metadata where the trick has been removed from the upstream data source (currently Google Sheets). This is done with the assumption that if a user has made an interaction with said Trick or Combo that they might still want to interact with it and keep their progression.

Assumptions / Requirements

In order to create a purposeful architecture we made some assumptions on what the application should / wants to be.

These assumptions are at the core of how the app should function and how it should be designed.

We made the assumption that the primary tasks of this app are to

  1. allow the user to manage their own tricks and combos
  2. allow the user to track their progression for tricks and combos.
  3. serve as a database of "official" tricks and combos, with their respective difficulties (potentionally to be used for competitions). → "official" tricks do not have to be editable by the user. They should stay in sync between all users.

If you feel like our assumptions are wrong or incomplete, please correct them. It is critical that we get them right.

Updading Predefined Data

Currently, a version number is used for updating predefined data. If the version number provided by a JS Object is bigger than the version in the Table, an update takes place. With our proposal we recommend the use of a hash-Function. This function will, for a given input, always return the same hash — with the idea that any change in the input will result in a completely different hash. This way, if the Input (predefined Trick JS Object) ever changes, evaluating them would result in a different hash.

We propose to store the Hash in a Dexie Table as a simple string. On startup, the predefined data gets run through the hash function. In case there is a difference between stored and computed hash all predefined rows are deleted from the Database and updated.

The following figure shows the steps taken during startup to check if the predefined tricks need to be updated, as well as the general procedure taken if an update is needed.

sequenceDiagram
    participant A as Start
    participant B as Version Table (Dexie)
    participant C as Trick Table (Dexie)

    A->>A: Load JS Object with <br>predefined tricks
    A->>A: Hash the object <br>(computed hash)
    A->>+B: Check if a hash is <br>already stored (stored hash)
    B-->>-A: 
    alt stored hash == computed hash
        note over A,C:  Stop. Already synced
    else else 
        A->>+C: Update predefined Trick Entries in DB<br>(see other Diagram for further info.)
        C-->>-A: 

        A->>B: Write Hash
    end 

Handling Deleted Predefined Data

As mentioned above, a new Type of Combo and Trick, the "Orphan" gets added. If a Predefined Trick or Combo gets deleted from the "source of truth" (the JS Object exported by the virtual module), it gets turned into an Orphan.

Here are the strategies we identified for dealing with deleted predefined tricks:

1. Orphan Tricks as 3rd Trick type

Predefined tricks that get deleted are turned into orphans. They then function the same way as user tricks, but are marked as Orphans in the UI.

2. Orphans as intermediary (in the limbo :D)

Predefined tricks that get deleted are turned into orphans. While in that state, they are excluded from any lists and searches. The user is prompted to either convert the tricks into User Tricks or to outright delete them.

3. Simply Delete Trick like it never existed (without Orphan Type)

This is probably undesirable because users could lose their progress or be confused as to why some tricks suddenly disappeared.

4. Autoconvert to User Trick (without Orphan Type)

This would skip the orphan stage and simply convert any to-be-deleted predefined trick into a user trick. This is a very simple solution with the downside that the user might be confused as to why there is a new trick in the list of tricks that were explicitly defined by them.

Waldemar prefers Option 2. Julian prefers Option 1 and 4. All of them would work. We would love your input on what you deem best.

Here you can see an overview of the deletion process for all 4 options. Pasted image 20230328200509

How we came to this design

There were some other approaches considered, with the most promising being the use of JS-Objects that contain the predefined tricks. The Database-Instance would access that object instead of the actual indexed DB if it wanted to query predefined data. The big benefit of going that route is that "constant" predefined data is outside the database. We would not need to care about updating predefined entries in the Database. The big downside however is that we cannot query predefined tricks using a Dexie query. Another downside is also that we would also need extra logic to normalize access between two separate data sources with two separate access strategies.

Rationale

This design allows us to keep one "source of truth" — that's the JS Objects defining predefined tricks created based on the Google Sheets tables. By having everything in the Dexie DB we can make use of Dexie's querying functionality without needing to join data from different sources (e.g. separate userTrick and predefinedTrick tables).

In order to satisfy the requirement of serving as an "official" database of tricks, removing the functionality for users to edit predefined tricks allows for a consistent global state of predefined tricks. This way all users will always have the same "official" trick information which makes it easier to use for competitions.

In order to still allows users to track their progression and make custom notes on official and user-defined tricks we opted to create a new table, metadata. This table is decoupled from the main trick table to allow for easier updates. It also makes it more clear what is "predefined" and what is "custom" data for predefined tricks from a developer's perspective.

Related issues and open questions

If there are any plans to add any new features which would affect the database structure, now would be a good time address them.

Here are some open issues that come to mind:

Another question in case you like the proposed concept of user notes: Should they consist of a string, or of a list of strings?

bastislack commented 1 year ago

Assumptions / Requirements

You pretty much nailed it! Of course there will be additional requirements in the future (such as having user accounts and being able to share tricks and stuff like that) but the three requirements that you listed won't change anymore, I guess.

Updating Predefined Data

The process you described for updating tricks sounds good to me.

Handling Deleted Predefined Data

I sat down with @Felix-Hmpl and discussed about this. Some points we came up with: Basically, as you already said, approach 3 is not desirable as the user does not want tricks to disappear (probably even if there was no interaction with the trick until now). Approach 2 could be a bit intrusive, if we have a big trick list update and decide to delete 15 tricks the user will be annoyed to have 15 pop-up messages asking, whether to delete the trick or to convert it to a user trick. What we thought would be a combination of the approaches 1/4 and 2. When a trick is deleted it automatically is converted to a user trick (or orphan trick). When the trick is accessed for the first time after the update, that's when a pop-up is shown asking the user whether the trick should be deleted or converted to a user trick (probably the user should just see the options 'Delete' and 'Keep It'). I am not sure if the orphan type is really needed for this approach. If we can just implement this logic in the user tricks, then I think adding a new trick type is unnecessary code, but if the logic is a lot of code that fits better to the orphan type then we should use it. I am thinking about the single responsibility principle vs. don't repeat yourself principle here: if we don't use orphans the user trick class might get bloated with things that do not belong here. However, if the orphan tricks have a lot of duplicate code from the user tricks we should probably not use it (if inheritance only adds the necessary code then probably that would be the way to go?).

What do you think about the approach?

Some Remarks

A minor comment to the DB design: The optional keyword for technicalName and alias is the other way round (every trick has a technical name and some tricks have aliases).

For now it seems fine to me that the metadata is the same for tricks and combos. If this changes it still shouldn't pose a problem as we could just decide to not show certain fields in the UI for combos or tricks.

Since we are saying that users can't modify predefined tricks (only by using them as a template for a user trick) I don't see the necessity of having a composite key for the tricks and combos. The predefined trick ids start with 10000 and I don't see any user creating 10000 tricks for now (not even Ian :D ). So we could just keep it as it is for now without problems, I guess.

I have created some additional columns in the trick list. One will be used for #197, 'dateAdded'.

Two other columns are one way to deal with #171. We have too many tricks now to show all of them on the main screen, especially tricks that pretty much noone cares about. So I added a bool column 'isImportant' and a column 'isVariationOf'. If a trick is not important at least one trick id has to be filled in the corresponding 'isVariationOf' field, this could be used to create little drop downs on tricks that have unimportant variations and show them if the user clicks on the drop down button.

I have not thought too much about #170 yet but one possibility would be (kind of related to #171) to allow an array of start and end positions to be used, where the first entry in the array is used as the main combination of positions and the other entries show other possible variations. But I think this can be handled in a later release, for now we have more important things to work on.

Let me know what you think and feel free to correct me with things that I got wrong.

WaldemarLehner commented 1 year ago

Haven't talked to Julian about this yet, so here are just my 2ct:

Updating Predefined Data

I see some mentions of inheritance. I don't see how that is relevant. Any logic should be pretty much Trick/Combo-Type agnostic. No inheritance takes place. DRY is not violated.

Your mentioned approach works for me. I would still argue that the orphan Type is helpful here. How would you track of a Trick has been "orphaned"?

Some Remarks

If this changes it still shouldn't pose a problem as we could just decide to not show certain fields in the UI for combos or tricks.

The predefined trick ids start with 10000 and I don't see any user creating 10000 tricks for now (not even Ian :D ).

The Issue here is abandoning a clean data structure. A good database should not rely on "magic numbers" or have some hidden meaning behind values. But maybe I am overdoing the entire engineering part :D

I would like you to take a look at this from the point of view of a new Dev. I would argue it is easier to infer if a DB entry is a User-Entry of a Predefined Entry with the composite key, instead of having a magic number (in this case, 10_000).

I don't see any user creating 10000 tricks for now (not even Ian :D ). While I agree, I would argue that it is a bad decision to limit the max count like that. The database should not restrict how many tricks a user can create.

So I added a bool column 'isImportant' Could go into detail on how isImportant would be handled? What is the default value? When does the value change? How does isImportant affect search results?

where the first entry in the array is used as the main combination of positions and the other entries show other possible variations

I don't like this because of the same issues mentioned above (e.g. first entry is implied to be primary) We could have a startPosition and endPosition, and an option alternativeStartPositions and alternativeEndPositions . Hmm.. but then also… having two fields for that is also rather ugly. @JulianDietzel, what do you think? :D

I guess it boils down to: Does the start Position need to be queried? Alternatively… Another Table could be created that defines all possible alternative combinations.

interface Table {
  id: number // PK, autoincrement
  trickId: number // Part of Composite FK; 
  trickType: TrickType // // Part of Composite FK;
  startPosition: Position
  endPosition: Position
  difficultyOverride?: Difficulty // Just a thought :P  
}

so something like


id trickId trickType startPos endPos difficultyOverride
0 2351 UserTrick SOFA KOREAN
1 2351 UserTrick SOFA SOFA 4
2 2351 UserTrick SOFA BELLY 6
3 2351 UserTrick SOFA CHEST 5

would define 4 alternative Start / Stop Positions

Sorry if the thing is a bit incoherent. :P

JulianDietzel commented 1 year ago

Handling Deleted Tricks

I like your proposal of showing the tricks like they are user tricks in the trick list but then prompting the user about how to handle them when the open it.

On the question of whether the Orphan-Type is needed: Firstly I think there is a small bit of confusion around what the Orphan-Type actually is: @WaldemarLehner and I intended for it to only be an Enum, not a full class. There would be a class called, for example, Trick and this class then has one attribute of type TrickType, which indicates whether the trick is a User-, Predefined- or Orphan-Trick. This way no special logic needs to be (re)written for the Orphan tricks. Adding the option to the TrickType enum would simply offer a way to show that the trick is now neither predefined anymore nor a proper user trick yet. Therefore this should not violate the DRY or single responsibility principle, form what I can see.

This was the best way to store the information that a trick is in this kind of "limbo", that we could come up with. Repeating Waldemars question:

How would you track of a Trick has been "orphaned"?

Some Remarks

weberax commented 1 year ago

Handling Deleted Tricks

I like the idea of combining option 1 and 2 but would insist on the extra orphan type and would suggest to rename it to something like "archived Trick". Like this we could exclude them of the main Tricklist by default and show them depending on some filters... (only archived, all tricks including archived ...) This would be similar like we handle this now with deleted tricks, to not break anything when deleting a trick (like the ComboDetails of a combo including that trick) we just mark them as deleted and down't show them anymore in the TrickList.

Composite Key / Compound Index

Nice, I like it. The >1000 was supposed to be only tempory.

Doubles, Variations, Different starting/ending Possition

Changing the Endpossiton of a trick makes it to a variation of that trick in my eyes, same as doubles... I guess we could combine all these 3 things under Trick-Variations, either in the same database table or a separate one.

To handle that nicely in the google sheet, I would suggest to:

  1. have a different document including all the "special" variations, which will have a different description or different video link, etc.
  2. add a Variation column to the document of the main tricks, which includes specific keywords (with separators) like *[Number] for including double, triples, quads ,... to up this number diff[Number] a separate difficulty [position]->[position] different ...

    What are good separators for this? (We would need to separate text into a list, containing lists)

  3. store variations to an extra database table, not containing full trick objects but only the changed values from the original trick, inheriting the other attributes
  4. when parsing the documents automatically create the variations from the keywords of 3.
  5. only linking the variations to the "orginal" trick. This would make it more expensive querying for variations but also easier to handle user added variations

This is just out of the top of my head, feel free to think trough all that again ;)

Dexie?

Now when we do this whole refactoring we could also consider changing to different library then Dexie... I don't know what alternatives there are, but one problem we always had with Dexie, was the lack of a simple relational database.

Or should we also consider a different storage option? How could we get more persistence?

bastislack commented 1 year ago

On the question of whether the Orphan-Type is needed: Firstly I think there is a small bit of confusion around what the Orphan-Type actually is:

Ok, thanks for the clarification :)

How would you track if a Trick has been "orphaned"?

I think this is basically answered in your first very detailed post in the image describing the process of deleting tricks for the different options. First we store the ids of predefined tricks in the current version, then we check the ids of the updated version and whichever tricks are missing in the updated version will be turned into 'Orphan Tricks'. Whenever the user first visits this trick a pop-up will show up asking the user whether to delete or keep the trick. It will then either be deleted or turned into a user trick.

You also convinced me that it is not the best idea to go with the magic number 10.000 :D So let's use the composite key variant.

Alternatively… Another Table could be created that defines all possible alternative combinations.

This seems like a good option to me. I also like @weberax's approach for that. However, I am still not sure how to handle variations (showing them on the main trick list or not; creating separate tricks for them or just list them in the original trick if they are not important; which ones should exist in the app anyway?). Do you think this is a decision we can delay a bit or should we come up with a way to do it now?

Since the isImportant field seems to be always be described by variationOf.length == 0 I think the whole field could be droppend an only the variationOf field be kept.

While it's true for now that I only put entries in rows where isImportant == false I am not sure if it will stay like this. E.g. a Double Yoda Roll 180 could be a trick that deserves a spot on the main trick list but is also a variation of a Double Yoda Roll. Again still uncertain about the variations topic...

So if something funky should happen between tricks, the user can just add it as text and it shows up similar to a trick in the UI

That seems like an easy fix for the problem. Maybe I wouldn't say that a combo is made out of tuples but rather that we can use 'normal' tricks and something like temporary/quick/fill trick (don't know how to best name it). Not really different from a note but kind of handled like a trick (just without the link to a trick page and so on).

Or should we also consider a different storage option? How could we get more persistence?

After a quick search it looks to me like indexedDB is the best option for offline storage in PWAs and Dexie is the most well-maintained indexedDB wrapper as far as I know. So maybe we should just stay with Dexie. But if you find any better alternatives we can also change this.

JulianDietzel commented 1 year ago

@WaldemarLehner and I looked at the recent suggestions and updated our old graphics accordingly:

db_schema drawio(4)

Untitled_Diagram drawio(1)

Changes

Open Questions

Others

This is what we have for now. Let us know what you think or if we missed anything (for which there is a decent chance).

bastislack commented 1 year ago

Nice, it looks really good!

  1. I like the way you want to handle variations.
  2. The Video class is a good idea
  3. Archived sounds good
  4. Official and user defined sounds good
  5. Splitting the enum sounds good.
  6. DateAddedEpoch sounds good.
  7. showInSearchQueries is good.

Open Questions:

  1. Multiple videos per trick seems like a good idea.
  2. We could use 'Stickable' instead of 'Entity'. Because we can stick tricks and combos ;)
  3. We decided to not include such generic tricks in the app for now.
  4. I think Dexie will do for now.
JulianDietzel commented 9 months ago

The database of the rewrite will be designed in accordance to this discussion. There will therefore be no change to the database of the current version of the app. It will be rewritten entirely as part of the complete rewrite of the app.