Skip to content

#3271: club account and product purchase other reason now a model#3281

Merged
wavehassman merged 33 commits intofeature/finance-redesignfrom
#3271-rerun-schema-changes
Apr 4, 2025
Merged

#3271: club account and product purchase other reason now a model#3281
wavehassman merged 33 commits intofeature/finance-redesignfrom
#3271-rerun-schema-changes

Conversation

@wavehassman
Copy link
Copy Markdown
Contributor

@wavehassman wavehassman commented Mar 6, 2025

Changes

I made Club_Account and Reimbursment_Product_Other_Reason models. This caused a bunch of errors in the frontend and backend, so I had to go and fix those.

Notes

In the migration, I am adding the existing allowedRefundSources to the account code to index code table, but not sure if that is right.

Test Cases

create index code get single index code get all index codes delete index code delete index code works create other product reason get single other product reason get all other product reasons delete other product reason

Screenshots

account code form modal new RR form new RR product table new

Checklist

  • All commits are tagged with the ticket number
  • No linting errors / newline at end of file warnings
  • All code follows repository-configured prettier formatting
  • No merge conflicts
  • All checks passing
  • Screenshots of UI changes (see Screenshots section)
  • Remove any non-applicable sections of this template
  • Assign the PR to yourself
  • No yarn.lock changes (unless dependencies have changed)
  • Request reviewers & ping on Slack
  • PR is linked to the ticket (fill in the closes line below)

Closes #3271

@wavehassman wavehassman self-assigned this Mar 6, 2025
Copy link
Copy Markdown
Contributor

@Peyton-McKee Peyton-McKee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally i would refactor the migration to look something like this

Change account to indexCode in RR Model

Create the Index Code Table, Create the Other Reimbursement Reason Table,
Create new Admin user
Insert each cash and budget into index code table and use admin user id as the user who created
Insert each other reimbursement reason into reimbursement table with admin as the creator
iterate over every rr and if rr.account === indexCode.name, set rr.indexCodeId = indexCode.indexCodeId, yk, and do something similar for whatever uses other reimbursement reason

Copy link
Copy Markdown
Contributor

@dreifusjack dreifusjack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was only able to review the frontend and endpoints bc you know more than me on the db stuff. Overall everything looks great! Just added some small fixes. Also I noticed that the getAllIndexCode endpoint ensures the user is an admin or on the finance team, but not the getSingleIndexCode, just want to confirm that one should be validating and the other not. Same goes for the getSingleOtherR... and getAllSingleOtherR...

@@ -201,27 +202,27 @@ const getTotalAmountOwedForCashAndBudgetForSubmittedToSaboAndPendingFinanceTeam
);

const totalAmountOwedForCashSabo = submittedToSabo.reduce((acc, curr) => {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assuming other organizations won't have the exact same accounts as us, this will have to be reworked. You should make it another ticket but it def needs to be done

Copy link
Copy Markdown
Contributor Author

@wavehassman wavehassman Mar 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would someone go about this like high level? Trying to write up a ticket and am stuck

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh actually I didn't realize this was the manual file, don't worry about it


export const isAccount = (validationObject: ValidationChain): ValidationChain => {
return validationObject.isString().isIn([ClubAccount.BUDGET, ClubAccount.CASH]);
return validationObject.isString().isIn(['Budget', 'Cash']);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This only works for these two accounts. If there are more, we need to check against an org's list of accounts

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is now a model, I changed the create and edit reimbursement request endpoints to now take in indexCodeIds instead of the whole model, so this isn't even used now.

@wavehassman wavehassman requested a review from walker-sean March 29, 2025 20:36
walker-sean
walker-sean previously approved these changes Apr 3, 2025
@@ -201,27 +202,27 @@ const getTotalAmountOwedForCashAndBudgetForSubmittedToSaboAndPendingFinanceTeam
);

const totalAmountOwedForCashSabo = submittedToSabo.reduce((acc, curr) => {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh actually I didn't realize this was the manual file, don't worry about it

@@ -1,4 +1,4 @@
import { ClubAccount, ReimbursementRequest } from 'shared';
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This modal needs to be reworked to use non-predermined accounts and codes. You can make that a seperate ticket if you'd like

@wavehassman wavehassman merged commit 76382ae into feature/finance-redesign Apr 4, 2025
4 checks passed
@wavehassman wavehassman deleted the #3271-rerun-schema-changes branch April 4, 2025 00:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants