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

Show author changes in exercise objects #1052

Conversation

ImTheTom
Copy link
Contributor

@ImTheTom ImTheTom commented May 26, 2022

Proposed Changes

  • Create new abstract model called AbstractHistoryMixin. Used to represent extractions from historical records.
  • Adds a command to add to change author history for exercises and exercises bases

Please check that the PR fulfills these requirements

  • Tests for the changes have been added (for bug fixes / features)
  • Added yourself to AUTHORS.rst

Other questions

  • Placing it up for review early to see if I'm on the right track

Showcase

Exercise object no changes

{
    "id": 1,
    "uuid": "b83e3d85-a53d-4939-a61c-7baa2e94d358",
    "name": "Trizeps Seildrücken",
    "exercise_base": 659,
    "description": "<p>Stelle dich mit schulterbreitem Stand vor dem Kabelzug, beuge den Rücken sehr leicht nach vorn und greife das Seil. Ziehe nun nach unten, wobei die Ellenbogen sich während der ganzen Übung nicht von der Stelle bewegen sollen. Wenn die Arme ausgestreckt sind, drehe die Hände nach außen. Gehe langsam zur Ausgansposition zurück.</p>",
    "creation_date": "2013-05-05",
    "category": 8,
    "muscles": [
        5
    ],
    "muscles_secondary": [],
    "equipment": [],
    "language": 1,
    "license": 1,
    "license_author": "James",
    "variations": [
        1,
        63
    ],
    "author_history": []
}

Add to license history table

python manage.py change-author --author-name Tom --exercise-id 1

Exercise now shows

{
    "id": 1,
    "uuid": "b83e3d85-a53d-4939-a61c-7baa2e94d358",
    "name": "Trizeps Seildrücken",
    "exercise_base": 659,
    "description": "<p>Stelle dich mit schulterbreitem Stand vor dem Kabelzug, beuge den Rücken sehr leicht nach vorn und greife das Seil. Ziehe nun nach unten, wobei die Ellenbogen sich während der ganzen Übung nicht von der Stelle bewegen sollen. Wenn die Arme ausgestreckt sind, drehe die Hände nach außen. Gehe langsam zur Ausgansposition zurück.</p>",
    "creation_date": "2013-05-05",
    "category": 8,
    "muscles": [
        5
    ],
    "muscles_secondary": [],
    "equipment": [],
    "language": 1,
    "license": 1,
    "license_author": "James",
    "variations": [
        1,
        63
    ],
    "author_history": [
        "Tom"
    ]
}

python manage.py change-author --author-name Tom --exercise-id 1

{
    "id": 1,
    "uuid": "b83e3d85-a53d-4939-a61c-7baa2e94d358",
    "name": "Trizeps Seildrücken",
    "exercise_base": 659,
    "description": "<p>Stelle dich mit schulterbreitem Stand vor dem Kabelzug, beuge den Rücken sehr leicht nach vorn und greife das Seil. Ziehe nun nach unten, wobei die Ellenbogen sich während der ganzen Übung nicht von der Stelle bewegen sollen. Wenn die Arme ausgestreckt sind, drehe die Hände nach außen. Gehe langsam zur Ausgansposition zurück.</p>",
    "creation_date": "2013-05-05",
    "category": 8,
    "muscles": [
        5
    ],
    "muscles_secondary": [],
    "equipment": [],
    "language": 1,
    "license": 1,
    "license_author": "James",
    "variations": [
        1,
        63
    ],
    "author_history": [
        "Tom"
    ]
}

python manage.py change-author --author-name James --exercise-id 1

{
    "id": 1,
    "uuid": "b83e3d85-a53d-4939-a61c-7baa2e94d358",
    "name": "Trizeps Seildrücken",
    "exercise_base": 659,
    "description": "<p>Stelle dich mit schulterbreitem Stand vor dem Kabelzug, beuge den Rücken sehr leicht nach vorn und greife das Seil. Ziehe nun nach unten, wobei die Ellenbogen sich während der ganzen Übung nicht von der Stelle bewegen sollen. Wenn die Arme ausgestreckt sind, drehe die Hände nach außen. Gehe langsam zur Ausgansposition zurück.</p>",
    "creation_date": "2013-05-05",
    "category": 8,
    "muscles": [
        5
    ],
    "muscles_secondary": [],
    "equipment": [],
    "language": 1,
    "license": 1,
    "license_author": "James",
    "variations": [
        1,
        63
    ],
    "author_history": [
        "James",
        "Tom"
    ]
}

@rolandgeider
Copy link
Member

Looks good! This was easier than what I had thought, I will try to take a closer look later today or so.

I'm not sure whether we need to expose the complete history: if you change the description twice your name will appear twice and while we need to keep this data internally, it's not so useful for consumers over the api.

@ImTheTom
Copy link
Contributor Author

ImTheTom commented May 26, 2022

Yeah I can see the advantage of having just the distinct authors and then potentially add new endpoints to fetch indepth information about an exercise history.

Another improvment to the MR would be to have these authors on all exercise API endpoints that it would make sense on. I guess that would be any already with the original license author on it.

@rolandgeider
Copy link
Member

I'm also thinking we could move license_author_history to a class, then we could easily expose the authors for all models that have a history

@ImTheTom
Copy link
Contributor Author

Yeah sure. Will be easier to maintain. I'll close this off and go with that approach.

@ImTheTom ImTheTom closed this May 27, 2022
@rolandgeider
Copy link
Member

You don't need to close this or, Just push your changes

@ImTheTom
Copy link
Contributor Author

Hey, would this be more suitable.

@rolandgeider
Copy link
Member

wouldn't the new LicenseAuthorHistory more or less duplicate some of the logic that the history plugin already does? What I meant with a class was to write a simple mixin that we could then just plug in in all the models that have a history.

class HistoryMixin():
    @property
    def author_history(self):
        ...
        return {}
        
        
class Exercise(AbstractLicenseModel, models.Model, HistoryMixin):
    ....

@rolandgeider
Copy link
Member

I should also finally fix the tests in the crowdsourcing branch, it's really annoying having constantly red checks!

@ImTheTom
Copy link
Contributor Author

Oh yeah I'll look at that approach 👍.

@ImTheTom
Copy link
Contributor Author

I think it's ready again for an initial review before proceeding :).

@property
def author_history(self):
out = []
for history in self.history.all():
Copy link
Member

Choose a reason for hiding this comment

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

can you make the list unique? Perhaps casting it to a set is enough, we don't care about the order

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah sure. I'll continue expanding out using this as a base.

@ImTheTom ImTheTom marked this pull request as ready for review June 5, 2022 00:36
@ImTheTom ImTheTom changed the title Draft: Show author changes in exercise objects Show author changes in exercise objects Jun 5, 2022
@ImTheTom
Copy link
Contributor Author

ImTheTom commented Jun 12, 2022

Hey man,

Is there anything else you would like from this change?

or if I even did the change you wanted correctly.

@rolandgeider
Copy link
Member

Hi! I just haven't found any time, but this looks ready :)

@ImTheTom
Copy link
Contributor Author

Yeah all good :)

@rolandgeider rolandgeider merged commit 1179ab2 into wger-project:feature/exercise-crowdsourcing Jun 14, 2022
@ImTheTom ImTheTom deleted the show-exercise-history branch June 23, 2022 07:11
@rolandgeider rolandgeider linked an issue Jul 21, 2022 that may be closed by this pull request
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.

Authors should not be a single field
2 participants