- 
                Notifications
    You must be signed in to change notification settings 
- Fork 6
Feat/derived achievements in important information #1090
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
base: master
Are you sure you want to change the base?
Feat/derived achievements in important information #1090
Conversation
aab1189    to
    d52992e      
    Compare
  
    | // Thus, the line below ensures that only the current or subsequent achievement steps are created, while others are automatically bypassed. | ||
| // Note: +1 is added because the index is 0-based, while the groupOrder is 1-based. | ||
| if (nextStepIndex + 1 < currentTemplate.groupOrder) { | ||
| if (templatesForGroup[nextStepIndex].groupOrder + 1 < currentTemplate.groupOrder) { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no idea if this will break anything. Need to check.
c6e3d33    to
    0cfdc3f      
    Compare
  
    e3200d3    to
    b1e29e1      
    Compare
  
    b1e29e1    to
    32415fc      
    Compare
  
    32415fc    to
    8ad1771      
    Compare
  
    af9e8d0    to
    723cc50      
    Compare
  
    723cc50    to
    8f67f13      
    Compare
  
    8f67f13    to
    bc9af4a      
    Compare
  
    bc9af4a    to
    5150828      
    Compare
  
    5150828    to
    3e254be      
    Compare
  
    3e254be    to
    f1252a5      
    Compare
  
    f1252a5    to
    dbfb1f0      
    Compare
  
    | You can test it here https://user-app-revert-achieve-m8qo7z.herokuapp.com/start This is the frontend PR | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall!
        
          
                common/achievement/derive.ts
              
                Outdated
          
        
      | context: ctx, | ||
| recordValue: null, | ||
| achievedAt: new Date(), | ||
| // achievedAt: hasRequest || achievement ? new Date() : null, | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the comment looks correct?
|  | ||
| async function derivePupilMatching(user: User, pupil: Pupil, result: achievement_with_template[], userAchievements: achievement_with_template[]) { | ||
| const hasRequest = pupil.openMatchRequestCount > 0; | ||
| const successfulScreenings = await prisma.pupil_screening.findMany({ | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.count(...) might be faster.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch!
|  | ||
| async function deriveStudentMatching(user: User, student: Student, result: achievement_with_template[], userAchievements: achievement_with_template[]) { | ||
| const hasRequest = student.openMatchRequestCount > 0; | ||
| const successfulScreenings = await prisma.screening.findMany({ | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
| }); | ||
| const derivedTemplates = deriveAchievementTemplates(userAchievements[currentAchievementIndex].template.group); | ||
| achievementTemplates.push(...derivedTemplates); | ||
| achievementTemplates.sort((left, right) => left.groupOrder - right.groupOrder); | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks strange - shouldn't it sort by group AND groupOrder? Or why by groupOrder?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because in this case all achievements in the list are of the same group
dbfb1f0    to
    550600b      
    Compare
  
    550600b    to
    a816211      
    Compare
  
    2b8e559    to
    16669e1      
    Compare
  
    16669e1    to
    308bdaa      
    Compare
  
    
No description provided.