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

Drobert fetch intra #48

Merged
merged 30 commits into from
Aug 29, 2023

Conversation

PukieDiederik
Copy link
Collaborator

This implements a command sync_db (with sub commands) which will fetch data from the 42 API and save it in our database.
The subcommands are the following:

  • user
  • skill
  • project
  • cursus
  • relations - Updates all the relations in the database
  • all - Runs the commands above

There is also a new module for fetching data from the 42 api. Its split into 2 classes (although the AuthApi42 should not really be used at the user end): AuthApi42 & Api42. Api42 is our interface for the user. This has a get command which will fetch data from the api using the GET request method.

The models.py file also got some improvements. Every model now has a __str__ & update functions. For the models that already had a __str__ function, they have been updated to be a more uniform in style. The update function updates/creates a model based on json data (What we got from the api).

Our command uses the logging (built-in) library. This will output to a logfile (unless told otherwise by the --no-logfile option) and the console. The output can be changed using the (built-in) -v option.

This should hopefully resolve the following issue(s):

Copy link
Owner

@pulgamecanica pulgamecanica left a comment

Choose a reason for hiding this comment

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

Its an absolute masterpiece I like the coding style and overall it's a very well done job.

I left some comments among the most important things to review are:

  • Encapsulation and advanced python concepts
  • PEP8 naming conventions for Class attributes and Class instance attributes
  • Review a little typo I found all ==> filter
  • Find out what response we get with 500 so we can handle it correctly
  • Discuss inheritance, pros and cons, consider this possibility

We should schedule a meeting to discuss this amazing progress ;)
GL! Keep it up 🥂

backend/portfolio42_api/management/api42/api42.py Outdated Show resolved Hide resolved
backend/portfolio42_api/management/api42/api42.py Outdated Show resolved Hide resolved
backend/portfolio42_api/management/api42/api42.py Outdated Show resolved Hide resolved
backend/portfolio42_api/management/api42/api42.py Outdated Show resolved Hide resolved
backend/portfolio42_api/management/api42/api42.py Outdated Show resolved Hide resolved
backend/portfolio42_api/management/commands/sync_db.py Outdated Show resolved Hide resolved
backend/portfolio42_api/models.py Outdated Show resolved Hide resolved
backend/portfolio42_api/management/api42/api42.py Outdated Show resolved Hide resolved
@pulgamecanica pulgamecanica added the api API problems, or improovements label Jul 25, 2023
@pulgamecanica pulgamecanica linked an issue Jul 25, 2023 that may be closed by this pull request
@PukieDiederik
Copy link
Collaborator Author

Made the changes that you suggested.

@pulgamecanica
Copy link
Owner

We should add to our wiki how the commands work, do you want to take over that? I can also do it, let me know 🗡️

Copy link
Owner

@pulgamecanica pulgamecanica left a comment

Choose a reason for hiding this comment

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

Awesome work, I am glad to see that you've progressed so much, I think this will help you a lot for the future, and working with Python is always a nice experience.

We just need to polish 💅 everything and it will be good to merge! ⏫

backend/portfolio42_api/management/api42/api42.py Outdated Show resolved Hide resolved
backend/portfolio42_api/management/api42/api42.py Outdated Show resolved Hide resolved
backend/portfolio42_api/management/commands/sync_db.py Outdated Show resolved Hide resolved
Copy link
Owner

@pulgamecanica pulgamecanica left a comment

Choose a reason for hiding this comment

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

Awesome! It's looking very nice.

We can discuss how to setup cron with docker later ;D
So it can run daily or once every two days.

We want to mention this with Vitor, so we have clear objectives for the next evaluation.

Cheers, happy summer! 🏖️

@pulgamecanica pulgamecanica merged commit 17b2344 into pulgamecanica:main Aug 29, 2023
1 check failed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api API problems, or improovements
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

ptapi42 Setup for rate limit and use with our commands CronJob [Architecture] - Django Admin
2 participants