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.

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

20907 - EFT Pay outstanding balance #2872

Merged
merged 5 commits into from
Jul 4, 2024
Merged

Conversation

ochiu
Copy link
Collaborator

@ochiu ochiu commented Jun 18, 2024

Issue #:
bcgov/entity#20907

Description of changes:

  • check for outstanding balance when switching to BCOL and initiate payment flow as needed
  • Outstanding amount owing view
  • Completing payment details view

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the sbc-auth license (Apache 2.0).

@ochiu
Copy link
Collaborator Author

ochiu commented Jun 18, 2024

Outstanding balance
Complete Payment Details

@ochiu ochiu marked this pull request as ready for review June 18, 2024 16:02
const isFromEFT = props.currentOrgPaymentType === PaymentTypes.EFT
if (!hasOutstandingBalance) {
bcOnlineDialog.value.close()
} else if (hasOutstandingBalance && isFromEFT) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

} else if (hasOutstandingBalance && isFromEFT) { -> } else if (isFromEFT) {

@@ -1,3 +1,5 @@
@import 'theme.scss';
Copy link
Collaborator

@seeker25 seeker25 Jun 18, 2024

Choose a reason for hiding this comment

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

I noticed some parts you imported Theme as well as actions below

Maybe only need actions.scss with this line?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

removed, don't actually need this import

align-items: center;
}
.payment-type-label {
display: flex;
Copy link
Collaborator

Choose a reason for hiding this comment

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

there are Vuetify classes for these

Copy link
Collaborator

Choose a reason for hiding this comment

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

margin-bottom: 12px !important;

.payment-method-card-text {
display: flex;
Copy link
Collaborator

Choose a reason for hiding this comment

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

there are Vuetify classes for these

Copy link
Collaborator

Choose a reason for hiding this comment

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

}
}
.payment-method-description{
display: flex;
Copy link
Collaborator

Choose a reason for hiding this comment

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

there are Vuetify classes for these

Copy link
Collaborator

Choose a reason for hiding this comment

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

@@ -831,6 +831,11 @@ export const useOrgStore = defineStore('org', () => {
setCurrentOrganizationPADInfo(undefined)
}

async function createOutstandingAccountPayment () {
const response = await PaymentService.createOutstandingAccountPayment(state.currentOrganization.id)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be moved to the service? do we care about response.data? in the store?
probably not..

Copy link
Collaborator

Choose a reason for hiding this comment

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

This doesn't do it with async, but I think we should make our services a bit more complicated (which should simplify stores):

https://github.com/bcgov/business-edit-ui/blob/2d76fb039f1dd6393574da807cbde30ef9a3e786/src/services/auth-services.ts#L60

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Makes sense, this would help the component logic to be more clean and less repetitive. I will be in this code soon doing EFT --> PAD screens probably. I will try to move the EFT calls out to the service when I do that.

Copy link
Collaborator

@seeker25 seeker25 left a comment

Choose a reason for hiding this comment

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

Looks good, well done

@seeker25
Copy link
Collaborator

Code quality issue on the unit test? I've seen this one before.. lol can't remember if the suggestion fixes it

Copy link

sonarqubecloud bot commented Jul 3, 2024

@ochiu
Copy link
Collaborator Author

ochiu commented Jul 3, 2024

Will merge after PR #1573 is in

@ochiu ochiu merged commit 12edcbc into main Jul 4, 2024
5 of 6 checks passed
@ochiu ochiu deleted the 20907-EFT-Outstanding-Balance branch July 4, 2024 20:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants