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

🧹 Move public_profile code from app.py to auth.py #2462

Closed
TiBiBa opened this issue Apr 7, 2022 · 8 comments · Fixed by #5644
Closed

🧹 Move public_profile code from app.py to auth.py #2462

TiBiBa opened this issue Apr 7, 2022 · 8 comments · Fixed by #5644

Comments

@TiBiBa
Copy link
Collaborator

TiBiBa commented Apr 7, 2022

Description
Currently we store the public profile related POST functions in app.py to ensure we can push achievements to the users. We are unable to perform this action in auth.py due to circular imports. However, with the use of requests we can simply make a POST from anywhere on the website, not being restricted by any imports. We use the same structure for when we are retrieving achievements on the front-end.

@TiBiBa TiBiBa self-assigned this Apr 7, 2022
@TiBiBa
Copy link
Collaborator Author

TiBiBa commented Aug 26, 2022

Still struggling a bit on what would be the best approach to fix this. By far the easiest solution would be to simply remove the achievement, but this is not the desired solution. Shortly stated this is caused by a bad dependency implementation long ago (my bad...). We have to decide if re-writing the achievements structure is worth if or if we want to use a "bucket-fix" as described above.

@Felienne
Copy link
Member

Hi @jpelay Another old issue to look into, I am not sure this is still relevant?

@Felienne Felienne moved this to ToBeDiscussed in Hedy organization board Sep 23, 2023
@Felienne Felienne changed the title [CLEAN-UP] Move public_profile code from app.py to auth.py [CHORE] Move public_profile code from app.py to auth.py Sep 23, 2023
@Felienne Felienne moved this from ToBeDiscussed to Long Term in Hedy organization board Sep 27, 2023
@Felienne
Copy link
Member

Hi @jtwaleson! Here is another issue about cleaning up the code base that you might enjoy working on? (no rush of course, I know your already have 2 other issues to work on, but since we are cleaning up, we are also looking whether we have people to take on older open issues)

@jtwaleson jtwaleson self-assigned this Oct 16, 2023
@Felienne Felienne changed the title [CHORE] Move public_profile code from app.py to auth.py 🧹 Move public_profile code from app.py to auth.py Feb 7, 2024
@Annelein
Copy link
Contributor

Description Currently we store the public profile related POST functions in app.py to ensure we can push achievements to the users. We are unable to perform this action in auth.py due to circular imports. However, with the use of requests we can simply make a POST from anywhere on the website, not being restricted by any imports. We use the same structure for when we are retrieving achievements on the front-end.

@Felienne is this still the current state? I haven't really done something like this yet, so not sure if I can easily implement 'requests'?

@Felienne
Copy link
Member

Description Currently we store the public profile related POST functions in app.py to ensure we can push achievements to the users. We are unable to perform this action in auth.py due to circular imports. However, with the use of requests we can simply make a POST from anywhere on the website, not being restricted by any imports. We use the same structure for when we are retrieving achievements on the front-end.

@Felienne is this still the current state? I haven't really done something like this yet, so not sure if I can easily implement 'requests'?

I don't know to be honest! Maybe @jpelay knows? If not, surely @rix0rrr will!

@Annelein Annelein moved this from Long Term to ToBeDiscussed in Hedy organization board Apr 15, 2024
@rix0rrr
Copy link
Collaborator

rix0rrr commented Apr 15, 2024

Thanks for the confidence, but to be honest I'm not quite sure either 😅

What we have to go on is this:

Move public_profile code from app.py to auth.py

So apparently there is some endpoint code that has to do with public profiles, that should have been in auth.py but wasn't put there because of:

due to circular imports

Probably there is a 3rd file somewhere that currently imports auth.py, but also that auth.py would need to import in order to do public profile-related work.

That's why the public_profile code was put into app.py, because nothing imports app.py.

However, with the use of requests we can simply make a POST from anywhere on the website

This is the part that confuses me the most.

requests is a Python library that we use to make HTTP requests to other websites: https://requests.readthedocs.io/en/latest/

The only thing that is making HTTP requests currently is:

  • The frontend to the backend.
  • The backend to services like Dynamo and S3 and Mailchimp.

Since requests is a backend library (Python), the only thing I can think of that @TiBiBa had in mind here was the backend making HTTP requests to itself? But that seems unnecessary, you can also reorganize the code.

Bottom line: I'm also not quite sure what was meant here.

@rix0rrr
Copy link
Collaborator

rix0rrr commented Apr 15, 2024

Aha it probably has to do with this code:

image

https://github.com/hedyorg/hedy/blob/main/app.py#L2526

@rix0rrr
Copy link
Collaborator

rix0rrr commented Apr 15, 2024

I think the point is to move this function:

@app.route('/auth/public_profile', methods=['POST'])
@requires_login
def update_public_profile(user):

To auth.py, by untangling some dependencies.

@Felienne Felienne moved this from ToBeDiscussed to In Progress in Hedy organization board Apr 16, 2024
@mergify mergify bot closed this as completed in #5644 Jul 31, 2024
@mergify mergify bot closed this as completed in 3f9dd64 Jul 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: In Progress
Development

Successfully merging a pull request may close this issue.

6 participants