-
Notifications
You must be signed in to change notification settings - Fork 6
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
Senior project release: 04/10/2024 #1034
base: sp-release-new
Are you sure you want to change the base?
Conversation
to test the virtual machine from senior project branch
change column name
…ordon-360-api into housing-add-roommate
…ordon-360-api into housing-add-roommate
…ordon-360-api into housing-add-roommate
Housing Lottery Add Applicants and Preferred Hall
hl-update-database
update string to Json array
update and get due date
}; ; | ||
await context.Applicant.AddAsync(newApplicant); | ||
} | ||
} |
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.
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 logic seems fairly simple and straightforward to me. I'm not sure it needs to change.
var myDueDate = context.Config.FirstOrDefault(d => d.Key == "housing_lottery_due_date"); | ||
myDueDate.Value = dueDate; | ||
await context.SaveChangesAsync(); | ||
} |
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 code here to update the row of housing_lottery_due_date
is not working. It cannot save the new due date. When I troubleshoot with break points, I found that it was able to update the myDueDate.Value
on line 646, but seems not be able to save it on line 647. By the way, it works if I drop the row and insert the new row, but it doesn't work if I update the value like this.
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.
Do you have an error message that you see?
Hi @EjPlatzer and @bennettforkner! I'm very sorry that I seem to have merged the previous senior project release PR somehow without realizing it, so I have to open this one. I resolved the requested changes you made in that PR, so I'm looking for a new round of code review👀. I still have two questions though, which I marked above. Could you also help me with them? Thank you!! |
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 work so far. No global concerns in my review, just individual stuff in the files.
/// </summary> | ||
/// <returns></returns> | ||
[HttpGet] | ||
[Route("halls/traditionals")] |
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.
Design preference, it would be much simpler (and would technically follow rest resource specifications) here if you had one route for halls
and had a query string variable that determined which type of hall you wanted to return. That's not a blocking issue, but it would be a clearer API spec IMO.
if (!viewerGroups.Contains(AuthGroup.HousingAdmin)) | ||
{ | ||
return Unauthorized(); | ||
} |
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 should be done in StateYourBusiness (SYB)
[HttpDelete] | ||
[Route("lottery/remove_user")] | ||
public ActionResult<bool> RemoveUser() | ||
{ | ||
var username = AuthUtils.GetUsername(User); | ||
bool result = housingService.RemoveUser(username); | ||
return Ok(result); | ||
} |
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.
No security on this route? Is it in the service?
It should be handled by the controller/SYB, not the service.
if (!viewerGroups.Contains(AuthGroup.HousingAdmin)) | ||
{ | ||
return Unauthorized(); | ||
} |
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.
SYB
Gordon360/Services/HousingService.cs
Outdated
public IEnumerable<HousingPreferenceViewModel> GetUserPreference(string username) | ||
{ | ||
var email = context.ACCOUNT.FirstOrDefault(a => a.AD_Username == username)?.email; | ||
var applicationID = context.Applicant.FirstOrDefault(a => (a.Applicant1 == email) && (a.Active == 1))?.ApplicationID; | ||
var preference = context.Preference.Where(a => a.ApplicationID == applicationID).Select(ph => (HousingPreferenceViewModel)ph); | ||
return preference.ToArray(); | ||
} |
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.
As I noted in the controller, preferences are technically not scoped to a user, so I would recommend taking in an application ID and using that. It makes the method simpler and more reusable (modular).
Gordon360/Services/HousingService.cs
Outdated
public IEnumerable<HousingPreferredHallViewModel> GetUserPreferredHall(string username) | ||
{ | ||
var email = context.ACCOUNT.FirstOrDefault(a => a.AD_Username == username)?.email; | ||
var applicationID = context.Applicant.FirstOrDefault(a => (a.Applicant1 == email) && (a.Active == 1))?.ApplicationID; | ||
var preferredHall = context.PreferredHall.Where(a => a.ApplicationID == applicationID).Select(ph => (HousingPreferredHallViewModel)ph); | ||
return preferredHall.ToArray(); | ||
} |
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 with preferred halls not really being scoped to a user
calculate year instead of storing it
fix the due date submission issue
No description provided.