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

Split exercises from translations #448

Closed
12people opened this issue Dec 26, 2019 · 13 comments · Fixed by #583
Closed

Split exercises from translations #448

12people opened this issue Dec 26, 2019 · 13 comments · Fixed by #583
Milestone

Comments

@12people
Copy link

As it stands, each exercise item is tied to a specific language.

That leads to several issues:

  1. Primary and secondary muscles, images, equipment, and categories all have to be added and maintained separately for each translation.
  2. If fetching exercises that correspond to a user's language, only a very small subset is retrieved.
  3. If adding a new exercise, it's almost impossible to check whether the exercise doesn't already exist in another language. (One would have to search for the translations of all supported languages.)

Suggested fix:

Split translatable data from the exercise data type.

As such, the new object model could look like this:

  • Exercise
    • id
    • license_author
    • status
    • translatables
    • creation_date
    • uuid
    • license
    • category
    • muscles
    • muscles_secondary
    • equipment
  • ExerciseTranslatable
    • id
    • license_author
    • description
    • language
    • name
    • name_original
    • creation_date
    • license
@rolandgeider
Copy link
Member

Yeah, the current solution is anything but elegant. I'm currently getting the project and the dependencies up-to-date again, but I'm open to changing this

@harlenesamra
Copy link
Contributor

My team will work on this thank you!

@rolandgeider
Copy link
Member

I've been thinking, since the exercise model is referenced in so many places, it might be easier to leave exercise just be and add an ExerciseBase, then use that one when necessary. This also has the advantage that e.g. the exercise URLs will stay the same

@jcho17
Copy link
Contributor

jcho17 commented Dec 4, 2020

@rolandgeider Could you elaborate on the proposed ExerciseBase model? What information would it include? Also, would each of the existing Exercise models be associated to its corresponding ExerciseBase? Thanks!

@rolandgeider
Copy link
Member

Sure. I meant that instead of splitting the exercise models into an exercise and an exerciseTranslatable like 12people suggested, we "flip" it. The exercise model would contain the language specific data, and reference a new ExerciseBase with the common fields (category, equipment, etc.).

The migration would look something like this:

  • add exerciseBase
  • move the data from all current exercises to base model
  • remove these fields from exercises
  • use a map so that exercises which are the same reference the same base. Unless some of you know German, I would make the list and provide it to you

@harlenesamra
Copy link
Contributor

Thanks so much for the clarification! I don't think any of us are fluent in German so that list would be greatly appreciated.

@oliexdev
Copy link

I played a bit around with the REST API and I have the following notes:

  • It would be useful to allow PATCH and DELETE for the private endpoints (e.g. weightentry)
  • getting all available exercises you need several get calls. First of all you need all ID, then you have to fetch all exercises information.
  • the return json format is always a object with a list (in "result") but then you have always a nested class. Would be better if the list is directly returned.
  • as @12people already noted the translation data is too embedded
  • how can we make sure that an exercise is unique? For example if somebody create an exercise "push ups" and another one "push up" which are identical but the name is slightly different!?
  • Would be great if it is possible to detect changes in the exercises or weightentry, so a third party app doesn't need to check everything again!? (maybe that is impossible)

Just my initial thoughts. 🤔

@rolandgeider
Copy link
Member

Hi @oliexdev ,

DELETE is allowed, I'm not sure about PATCH, but I haven't explicitly disallowed it (or at least don't remember doing that).

There is a separate /exerciseinfo endpoint where the child models are returned as well (it's probably not clear that this exists)

And as for new exercises being too similar, there's already #551 for that :)

@oliexdev
Copy link

@rolandgeider If I try the DELETE operation I get {"detail":"Method \"DELETE\" not allowed."}. It is also not listed in the API browser or OPTIONS but maybe I do something wrong!?

Indeed I thought you need the excercise id for the exerciseinfo endpoint. Could you tell me what the UUID for? Is that an unique identifier for each exercise? Who and when is this generated?

In some description of an exercise there are HTML tags included, would be good if they would deleted before they stored in the database. The formatting should be done afterwards.

@rolandgeider
Copy link
Member

Sorry I had somehow missed your answer.

The UUID is just an ID that is used to uniquely identify the exercises, e.g. during databas migrations as it is possible that local instances have deleted or added their own exercises. The UUID is automatically generated during creation.

And as for the formatting, yes, that is not ideal. I had also thought of using markdown or such, but for the moment that works. Before saving this HTML, it is passed through bleach, which only allows tags from a whitelist

@rolandgeider rolandgeider added this to the 2.0 milestone Jan 8, 2021
@rolandgeider
Copy link
Member

Just merged the PR that split the translations. At the moment all the (REST) APIs have stayed the same, so the next steps will be to actually make translations easier

@oliexdev
Copy link

oliexdev commented Jan 9, 2021

@rolandgeider thanks for your answer could you verify if the DELETE operation works for you? Either I am doing something wrong or it is still not allowed.

@rolandgeider
Copy link
Member

It is actually not allowed, just checked. To avoid confusion I have changed it so that it doesn't say it supports it (but this is obviously a TODO for the future)

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 a pull request may close this issue.

5 participants