Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
#1978 - Request an Offering Change: Student View Request #2196
#1978 - Request an Offering Change: Student View Request #2196
Changes from 18 commits
8707332
30b7be9
cd7ee07
3256149
5f83c73
4421a3a
802bdd6
5684742
d365524
866cb8a
de02138
fe1cd55
1386b79
c67a4a5
84ccbbb
332a873
919ca6e
466f57b
6b2f130
e231129
a0437e5
1a19253
defece4
44c8612
0ab2cb7
0ec2d78
7d002cb
c353925
b7eeac1
1239af6
04ef5cc
15b23de
09b31aa
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 get your point, there is nothing wrong in creating a overridden method, but the reason for naming the methods for the process they do is what we are following as a standard. So it would be better to make them explanatory and explaing the paramaters wherever possible. Also the variable names are mentioned as proper applicationOfferingChangeRequestId rather than id, its again for the same purpose and even if the developer misses the comments on the top of the method the calling class will have an idea when we try to implement them.
Also this will create consistency of the existing code.
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.
rather than returning the same paramterers in 2 places, you can create a
const applicationOfferingDetails = {
status: request.applicationOfferingChangeRequestStatus,
applicationNumber: request.application.applicationNumber,
locationName: request.application.location.name,
activeOfferingId: request.activeOffering.id,
requestedOfferingId: request.requestedOffering.id,
reason: request.reason,
};
if (options?.applicationOfferingDetails) {
return applicationOfferingDetails;
}
return {
...applicationOfferingDetails,
id: request.id,
requestedOfferingDescription: request.requestedOffering.name,
requestedOfferingProgramId: request.requestedOffering.educationProgram.id,
requestedOfferingProgramName: request.requestedOffering.educationProgram.name,
assessedNoteDescription: request.assessedNote?.description,
studentFullName: getUserFullName(request.application.student.user),
};
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 we still need it?