Feature/onboarding tasks #454
Conversation
|
Thanks for contributing! |
There was a problem hiding this comment.
Left some initial comments and questions. I'm a little curious about future extensibility, e.g. if we have more reward-type features (basically acm passport), the way we're going right now demands having boolean flags on the user object. Thoughts on designing an alternative solution to handle this?
My main question is also how the onboarding reward is supposed to be "collected" since that was one of the main issues we were discussing back during acm passport ideation
services/UserAccountService.ts
Outdated
| @@ -269,7 +288,7 @@ export default class UserAccountService { | |||
| .activity(txn) | |||
| .logActivity(activity); | |||
|
|
|||
| return updatedUser; | |||
| return updatedUser.getFullUserProfile(); | |||
There was a problem hiding this comment.
what's the need to return full user profile here?
There was a problem hiding this comment.
uh not sure why i did that
| } | ||
| if (userProfile.onboardingCollected) { | ||
| throw new BadRequestError('Onboarding reward already collected!'); | ||
| } |
There was a problem hiding this comment.
Prior to returning, we should probably also log this in the activity table
There was a problem hiding this comment.
Do I just add the points bonus to activities or should I do the update to the user model as well?
There was a problem hiding this comment.
Points bonus should just be fine–can you check if we log to activity any time there's a user object edit? I know we do it for user access updates but if there's more instances of it besides that then we can log the update to the user model
|
I think the main reason I'm just hard-coding this achievement for now is because we wanted just to deliver this feature quicker while we have more discussions about actually how to actually design and implement an ACM passport system in the future. Especially since ACM passport still needs to be designed and there's still feedback and some other tasks to do on the backend side, I felt it's probably best just to do it this way first and redesign later. |
|
Sure, I agree that having users "claim awards" or manually try to cash in is probably a good idea for now. In any case, updating that sort of thing in the future can be more easily swapped out. If you really want this feature to be live by next week or something, then I suppose this change is fine. However, it does introduce significant tech debt–you'll now need some sort of script/migration to port over the data in this format to any new schema, then drop the column on the user table. Not ideal/good practice, you would need to have this run on every single user and generate x new rows in whatever future table holds this kind of data. Certainly not the end of the world computationally though. My recommendation would be to design out the new feature (at least a V1 that can support multiple kinds of rewards), then introduce it at the start of winter quarter since fall has already started and it's unlikely that many new users are onboarding to the portal right now. I would really recommend not letting unclean practices pile up in the codebase and then deal with future problems later on, but we can discuss how you feel about it. |
|
Ok that is a good point I forgot about that. But on the other hand I was kind of thinking of these rewards as separate from a future achievement system. Especially since some of the checks here (i.e updating, bio, resume, pfp) probably wouldn't be part of a future achievement system that primarily tracks events attended/points spent. Ideally we would probably create a single achievement system that tracks all of it but I'm not really sure about what that would look like. Also I kinda wanted to just get this feature pushed because it might be a while before we actually get to implementing an achievement system. There's still the feedback revamp to do and I was thinking we probably need to handle the updating packages stuff first since it seems like Typeorm has some big changes to figure out along with some eslint and aws changes. |
There was a problem hiding this comment.
In that case I'll leave it up to you, but it would be good to prioritize the design for this so that you can maybe look to implementing it in the next few months. If you want to move forward, feel free. I'll approve right now but check out my comment about the activity table and make sure to test this change in staging first. Also have the other backend devs take a look
Info
Closes 453.
Description
Added a column for onboardingSeen and onboardingCollected in the Users table. Adjusted private profiles to return information for these onboarding tasks.
Changes
Type of Change
expected)
workflows, linting, etc.)
If you've selected Patch, Minor, or Major as your change type, make sure to bump the version before merging in
package.json!Testing
I have tested that my changes fully resolve the linked issue ...
Checklist
package.jsonfile.Screenshots