Merging Completed Profile - #240
Conversation
…b.com/COS301-SE-2026/CodeClash into feature/restart-branch-profile-morgan
…y - completed, must be tested
…this as i am confused about which id is retrieved from user.id
…ct elo for other people but i have a backend failure
… profile view value so that user values can fit in profile and user provider values for providing league
Swe-yy
left a comment
Swe-yy
left a comment
There was a problem hiding this comment.
Please review the comments
|
|
||
|
|
||
| async getUserRank(userId: string): Promise<RankDTO | null>{ | ||
| return this.eloRepository.getUserRank(userId); |
There was a problem hiding this comment.
this is supposed to be in a controller not in a system
| const user_repo = new UserRepository(AppDataSource.getRepository(Users)) | ||
| const elo_repo = new EloRepository(AppDataSource.getRepository(EloRatings)) | ||
|
|
||
| const leaderboard_system = new LeaderboardSystem(elo_repo); |
There was a problem hiding this comment.
just pass the elo_repo to the controller
there's no need for the systems to be involved here
| return; | ||
| } | ||
|
|
||
| const rank = await service.getUserRank(userId); |
There was a problem hiding this comment.
just call the repo directly for simple getters
Use the service (not system) when responses need multiple repo calls and added processing
| } | ||
| } | ||
|
|
||
| async getUserRank(userId: string): Promise<RankDTO | null> { |
There was a problem hiding this comment.
this should be in the user repo because rank is in attribute of a user not of elo
There was a problem hiding this comment.
The reason this was in elo repository is because even though it is getting the user's rank, it requires elo calculations in order to get the rank and so if you look in the getUserRank function in the elo repository, the elo repository needs to be fetched so that those different operations in order to fetch and order elo can be done. I do understand it is confusing and a bit counter intuitive though.
There was a problem hiding this comment.
I don't understand. The elo is already calculated. What calculations are needed.
The elo repository can still be called by the leaderboard Service if it's needed
| setError(`Error: ${res.status} ${res.data}`); | ||
| } | ||
| }) | ||
| // await axios.get(url.concat('user/league'), { |
There was a problem hiding this comment.
remove these comments if they arent being used
| // } | ||
| // }) | ||
|
|
||
| if(elo >= 600 && elo < 1200){ //change this to "elo >= 600", the lowest is 600, but test_user has an elo of 0 so that needs to be changed then this should be changed |
There was a problem hiding this comment.
this can be converted to a map or switch statement
Please look into converting this
|
|
||
| try { | ||
| await axios.get(url.concat('user/rank'), { | ||
| await axios.get(url.concat('elo/rank'), { |
There was a problem hiding this comment.
why is rank an attribute of elo
|
|
||
| const Leaderboard = () => { | ||
| const { userData, topThree, isLoadingData, error, page, totalPages, nextPage, prevPage } = LeaderboardViewModel('earth'); | ||
| const { userData, topThree, isLoadingData, error, page, totalPages, setPage, nextPage, prevPage } = LeaderboardViewModel('earth'); |
There was a problem hiding this comment.
what is setPage for
if the page needs to be changes use navigation instead
There was a problem hiding this comment.
I did not create that variable and upon further inspection it seems this is Ntu's work and setPage specifically changes which users are shown on the leaderboard (different pages of users of differing rank), so i do believe this should be kept
| <Route path='/dashboard' element={<Dashboard/>}/> | ||
| <Route path='/profile' element={<Profile/>}/> | ||
| <Route path='/leaderboard' element={<Leaderboard/>}/> | ||
| <Route path='fr' element={<FinalResults/>}/> |
There was a problem hiding this comment.
please change this path to be more intuitive
| "axios": "^1.18.1", | ||
| "clsx": "^2.1.1", | ||
| "color-convert": "^3.1.3", | ||
| "jest": "^30.4.2", |
… requested of me - completed
…ystem is not used and the elo repository is passed straight through
|
nsmzoneli
left a comment
nsmzoneli
left a comment
There was a problem hiding this comment.
Pull from dev and conflict resolve before you make any changes.
Please remove "League -" Before the user's league in the profile, it becomes a bit redundant when it's made clear to the user that "Mercury" means their league from the dashboard. I'm going to assume the actual fetching of elo values works on your machine.
4 participants
Finally got the Profile to display user rank, as well as user league