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

Feature/BE-36: User Level #364

Merged
merged 6 commits into from
Dec 19, 2022
Merged

Feature/BE-36: User Level #364

merged 6 commits into from
Dec 19, 2022

Conversation

BElifb
Copy link
Contributor

@BElifb BElifb commented Dec 18, 2022

  • Created an API for calculating user level.
    image

  • Initially created a function inside User model. But then moved the logic to API function since the prior led to circular import error.

  • The level is calculated using number of art items and comments the user and user's visits to other art items and user profiles. (In varying degrees of importance)

  • Currently I chose a low threshold, since we don't have a lot of data, but we can update this later.

  • Tested through postman and admin site.

  • Documented with swagger.

@BElifb BElifb added Effort: Low This issue can be easily handled Priority: Medium This issue should be handled, if there isn't any high priority issue Status: Completed The issue is closed Coding The issue is related with coding Team: Backend issues related to backend labels Dec 18, 2022
@BElifb BElifb requested review from KarahanS and mumcusena December 18, 2022 13:53
@BElifb BElifb self-assigned this Dec 18, 2022
@BElifb BElifb mentioned this pull request Dec 18, 2022
4 tasks
@KarahanS
Copy link
Contributor

Correct me if I'm wrong but in order for someone's level to be calculated, he needs to make an API request, right? I think that would be better to update it regularly whenever the related user's profile is visited (when GET profile is called). I think, in order to do that, we have to use a function as you tried in your first attempt.
If it causes a circular import problem, this might help. I solved the same problem using one of the approaches given in that link while working on exhibitions (I was importing exhibitions from artitems and artitem from exhibitions due to the relationship between them). If trying to fix that increases the effort a lot, then we can use the API as it is, let us not let this problem block us.
Additionally, this API returns the level of the currently logged-in user. Won't a user be able to view another user's level?

@BElifb
Copy link
Contributor Author

BElifb commented Dec 18, 2022

  • You're right about the API call. I was thinking we could possibly add an apply for level2 button in the settings page.
  • I am not really happy with the button approach either, but I thought making the level calculation (with all of the database queries) everytime user visits the profile would create a lot of unnecessary overhead. A possible solution could be scheduled checks, but I didn't want to waste time learning how to.
  • About circular import, link you mentioned seems to offer a pretty clean solution, I could either use that or simply move the logic to profile API. But still not sure about the overhead this would create. Let me know what you think @KarahanS
  • And for seeing other users' level, the function I wrote is only meant to be used for upgrading a user's level. Other than that we keep the level information in is_level2 field for each user. Profile API already returns this, we can use it to display level in whatever format we choose in the frontend.

@KarahanS
Copy link
Contributor

About circular import, link you mentioned seems to offer a pretty clean solution, I could either use that or simply move the logic to profile API. But still not sure about the overhead this would create. Let me know what you think @KarahanS

By overhead, do you mean the time consumed by calculating user's level every time his profile is visited? I think, waiting for users to update their levels by clicking on a button is not really user-friendly. Also, I don't think couple of calculations will hurt the performance. Right now, the bottleneck in our application is the AWS itself (fetching images or uploading). So I don't really feel like anyone will complain about the time wasted by these calculations :D
It's up to you, but again, I feel like we shouldn't wait for users to update their levels manually.

@BElifb
Copy link
Contributor Author

BElifb commented Dec 19, 2022

  • Got django.core.exceptions.AppRegistryNotReady: Models aren't loaded yet. error, turns out needed to import models inside class.
  • Now the calculateLevel function of User is working and is called automatically whenever a user visits their own profile or updates.

@BElifb
Copy link
Contributor Author

BElifb commented Dec 19, 2022

  • Solved merge conflicts

@KarahanS
Copy link
Contributor

KarahanS commented Dec 19, 2022

Tested the API and the level calculation. Everything seems ok, thanks for the efforts. (Since we integrated the level calculation into profile, I can't see where new API will be used though)

@KarahanS KarahanS added the Approved This work is reviewed and approved by a team member label Dec 19, 2022
@BElifb
Copy link
Contributor Author

BElifb commented Dec 19, 2022

Perhaps, if we want to change when we check level update.

@BElifb BElifb merged commit 2e240f5 into master Dec 19, 2022
@BElifb BElifb deleted the feature/BE-36 branch December 19, 2022 17:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approved This work is reviewed and approved by a team member Coding The issue is related with coding Effort: Low This issue can be easily handled Priority: Medium This issue should be handled, if there isn't any high priority issue Status: Completed The issue is closed Team: Backend issues related to backend
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants