Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[#13064] Use AccountRequestUpdateRequest as parameter #13068

Merged
merged 6 commits into from
Apr 24, 2024
Merged

[#13064] Use AccountRequestUpdateRequest as parameter #13068

merged 6 commits into from
Apr 24, 2024

Conversation

Stain19
Copy link
Contributor

@Stain19 Stain19 commented Apr 20, 2024

Fixes #13064

@github-actions GitHub Actions
Copy link

Hi @Stain19, thank you for your interest in contributing to TEAMMATES!
However, your PR does not appear to follow our contribution guidelines:

  • Title must start with the issue number the PR is fixing in square brackets, e.g. [#<issue-number>]

Please address the above before we proceed to review your PR.

@ziqing26
Copy link
Contributor

ziqing26 commented Apr 20, 2024 •
edited
Loading

@Stain19 Hi please fix the lints as specified by the github action 'Component Test/lint'. Thanks!

@ziqing26 ziqing26 requested a review from jayasting98 April 20, 2024 06:14
@ziqing26 ziqing26 added the c.Task Other non-user-facing works, e.g. refactoring, adding tests label Apr 20, 2024
@weiquu weiquu mentioned this pull request Apr 20, 2024
Closed
@Stain19
Copy link
Contributor Author

Stain19 commented Apr 22, 2024

Changes Made

jayasting98
jayasting98 reviewed Apr 22, 2024
View reviewed changes
Copy link
Contributor

@jayasting98 jayasting98 left a comment

Choose a reason for hiding this comment

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

Just a nit.

src/web/services/account.service.ts Outdated
status: AccountRequestStatus, comments: string)
editAccountRequest(id: string, accountReqUpdateRequest: AccountRequestUpdateRequest)
: Observable<AccountRequest> {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is just a nit. Would it be possible to combine this into one line? So like

editAccountRequest(id: string, accountReqUpdateRequest: AccountRequestUpdateRequest) : Observable<AccountRequest> {

If I'm not wrong, the limit is 120, and this single line should only be 117, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changes Made!!
Apparently nothing went wrong, i just run the lint test 👍🏼

@jayasting98 jayasting98 added s.ToReview The PR is waiting for review(s) s.ToDiscuss The issue/PR is undergoing discussion/review by the core team and removed s.ToReview The PR is waiting for review(s) labels Apr 22, 2024
@Stain19 Stain19 requested a review from jayasting98 April 23, 2024 00:49
jayasting98
jayasting98 approved these changes Apr 23, 2024
View reviewed changes
Copy link
Contributor

@jayasting98 jayasting98 left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution!

@jayasting98 jayasting98 added s.FinalReview The PR is ready for final review and removed s.ToDiscuss The issue/PR is undergoing discussion/review by the core team labels Apr 23, 2024
cedricongjh
cedricongjh approved these changes Apr 23, 2024
View reviewed changes
Copy link
Contributor

@cedricongjh cedricongjh left a comment

Choose a reason for hiding this comment

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

LGTM!

@cedricongjh cedricongjh added s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging and removed s.FinalReview The PR is ready for final review labels Apr 23, 2024
@cedricongjh cedricongjh added this to the V9.0.0-beta.7 milestone Apr 23, 2024
@ziqing26 ziqing26 merged commit e738e25 into TEAMMATES:master Apr 24, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cedricongjh cedricongjh cedricongjh approved these changes

@jayasting98 jayasting98 jayasting98 approved these changes

Assignees

@Stain19 Stain19

Labels
c.Task Other non-user-facing works, e.g. refactoring, adding tests s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging
Projects
None yet
Milestone
V9.0.0-beta.7
Development

Successfully merging this pull request may close these issues.

Use AccountRequestUpdateRequest as parameter
4 participants
@Stain19 @ziqing26 @jayasting98 @cedricongjh