Updates hideCvv flag for edit payment methods and new payment methods. #1109

wjames111 · 2024-10-28T18:17:22Z

OKR: Implement real-time CVV validation on all transactions for Give App

wjames111 · 2024-11-04T22:05:58Z

@wrandall22 I'm having some difficulty with this one. I need to validate that the CVV is correct when paying with a saved card. I added a save payment method that is essentially just ripped from creditCardForm.component.js. I haven’t been able to get it working though which isn't surprising seeing as I'm not entirely sure what all is going on in that code block. Do you have any pointers for me/is this the right way of going about it?

wrandall22 · 2024-11-05T14:32:36Z

So, there are a few things.

You should probably make the changes in existingPaymentMethods instead of in paymentMethodDisplay since the first is only used in checkout and the latter is used in several places we don't want to capture cvv.
The validation is happening, but is not preventing the user from moving forward, which means we need to hook into the continue button on checkout step 2 and ensure that the validation passes there before moving forward.
The Angular way of re-using the cvv template/controller code is to create a directive and put it in both places. We have several directives in the codebase already which you can reference (even creditCardNumber has one). This would be a way to not copy/paste code and maintain it in two places.

wjames111 · 2024-11-05T15:25:54Z

@wrandall22 thank you this is really helpful. One question, do we only need the validators on the CVV? I would assume we need to ensure the CVV is also correct for the given credit card before allowing them to move forward. That was what I was trying to accomplish with the savePayment method, although it didn't appear to be working.

wrandall22 · 2024-11-05T15:31:08Z

The CVV is the only thing being entered, so is the only thing being validated in this situation. That validation can live in the directive.

wjames111 · 2024-11-05T15:37:18Z

Ahh okay that makes things a bit easier.

dr-bizz · 2024-11-08T15:29:30Z

Great work on this PR!

Just a note: before merging with staging, can you run yarn lint:write? This will ensure the lint issues are fixed or bring attention to code that needs to be changed. This way, your changes on staging will be deployed to the staging site.

dr-bizz · 2024-11-08T15:57:26Z

Removing the on staging label for now. Once everything is working on this branch, feel free to add it back

dr-bizz · 2024-11-08T15:58:08Z

It seems like it wasn't just you, there were some other changes from other PRs that caused staging to fail being deployed.

wjames111 · 2024-11-08T15:59:39Z

@dr-bizz thanks that make me feel better. Sorry about that, I didn't realize it was causing issues. It's not quite ready for staging either.

wrandall22 · 2024-11-08T16:03:47Z

You can turn it into a draft PR until it's ready if you want.

dr-bizz · 2024-11-08T16:03:55Z

It's all good. This is where amplify previews would come in handy

wjames111 · 2024-11-13T17:37:16Z

@wrandall22 is this more what you had in mind? Do you know where else the directive needs to be used apart from existingPaymentMethods?

wrandall22 · 2024-11-13T18:31:20Z

Hey Will, thanks for working on this. I was more thinking of putting this part of the HTML in the directive:

<div class="form-group" ng-class="{'has-error': ($ctrl.creditCardPaymentForm.securityCode | showErrors), 'is-required': !$ctrl.paymentMethod}">
          <label translate>{{'SEC_CODE'}}</label>
          <input type="text"
                 name="securityCode"
                 autocomplete="cc-csc"
                 class="form-control form-control-subtle"
                 ng-model="$ctrl.creditCardPayment.securityCode"
                 ng-required="!$ctrl.paymentMethod || $ctrl.creditCardPayment.cardNumber"
                 ng-attr-placeholder="{{$ctrl.paymentMethod && !$ctrl.creditCardPayment.cardNumber ? '***' : ''}}">
          <div role="alert" ng-messages="$ctrl.creditCardPaymentForm.securityCode.$error" ng-if="($ctrl.creditCardPaymentForm.securityCode | showErrors)">
            <div class="help-block" ng-message="required" translate>{{'CARD_SEC_CODE_ERROR'}}</div>
            <div class="help-block" ng-message="minLength" translate>{{'MIN_LENGTH_CARD_SEC_CODE'}}</div>
            <div class="help-block" ng-message="maxLength" translate>{{'MAX_LENGTH_CARD_SEC_CODE'}}</div>
            <div class="help-block" ng-message="cardTypeLength" ng-init="isAmex = $ctrl.cardInfo.type($ctrl.creditCardPayment.cardNumber) === 'American Express'">
              <translate ng-if="!isAmex">{{'LOCATION_OF_CODE_OTHER'}}</translate>
              <translate ng-if="isAmex">{{'LOCATION_OF_CODE_AMEX'}}</translate>
            </div>
          </div>
        </div>

Then using it in both creditCardForm and existingPaymentMethods

wjames111 · 2024-11-14T21:02:55Z

@wrandall22 is this a little more what you had in mind? I was initially trying to validate within the directive itself, but it ended up being easier to just use the directive for the template and use the validators that were already in place.

wrandall22 · 2024-11-14T21:05:59Z

Yep, this is the direction I was thinking.

wjames111 · 2024-11-14T21:07:20Z

@wrandall22 okay great! I'll fix it up.

src/app/checkout/step-2/step-2.component.js

wrandall22

I got part way through the review, this is a good start. I'm going to play around a little bit with the directive locally as well.

src/app/checkout/step-2/step-2.component.js

src/app/profile/payment-methods/payment-method/payment-method.component.js

src/app/profile/payment-methods/payment-methods.component.js

src/common/components/paymentMethods/creditCardForm/creditCardForm.component.js

src/app/checkout/step-2/step-2.component.spec.js

src/common/directives/creditCardCvv.directive.js

src/app/checkout/step-2/existingPaymentMethods/existingPaymentMethods.component.js

src/common/directives/creditCardCvv.directive.js

wjames111 · 2024-11-20T22:03:51Z

Thanks @wrandall22! I'll get started on these changes.

wrandall22 · 2024-11-25T21:13:03Z

src/app/checkout/step-2/existingPaymentMethods/existingPaymentMethods.component.js

+    if (this.selectedPaymentMethod?.['card-type'] && this.creditCardPaymentForm?.securityCode) {
+      const selectedUri = this.selectedPaymentMethod.self.uri
+      const storage = JSON.parse(this.sessionStorage.getItem('storedCvvs'))
+      const getSelectedCvv = storage ? storage[Object.keys(storage).filter((item) => item === selectedUri)] : false


If you use '' instead of false you don't need the conditional below.

Oh nice I like that!

wrandall22 · 2024-11-25T21:15:02Z

src/app/checkout/step-2/existingPaymentMethods/existingPaymentMethods.component.spec.js

+
+        it('should add securityCode viewValue from sessionStorage', () => {
+          self.controller.creditCardPaymentForm.securityCode.$viewValue = '123'
+          self.controller.selectedPaymentMethod = { 'card-type': 'Visa', self: { type: 'cru.creditcards.named-credit-card', uri: '/paymentmethods/crugive/giydsnjqgi=' }, selectAction: 'some uri' }


This line is a bit too long, let's break it up to multiple lines.

dr-bizz

Thank you for thinking the error on the checkout. I have review the code again and made some comments, but they are only small things.

I did test the security code on the checkout and found it didn't matter what value I added, it always allowed me to continue. Should that be the case, or are you just taking the input on step 2, and then checking it when the user submits the checkout?

dr-bizz · 2024-11-26T14:01:27Z

src/app/checkout/step-2/step-2.component.js

@@ -116,7 +116,10 @@ class Step2Controller {
    }
  }

-  getContinueDisabled () {
+  isContinueDisabled () {
+    if (this.selectedPaymentMethod?.['card-type'] && typeof this.isCvvValid !== 'undefined' && !this.isCvvValid) {


so if isCvvValid is undefined we enable the continue button?

Is it that if there are no CC to ensure they are valid, we don't disable the button, so the user isn't blocked.

I see on line 129 we ensure a payment method is selected.

I may be able to take that out now but initially I added typeof this.isCvvValid !== 'undefined' so that it's not disabled before enableContinue gets called.

dr-bizz · 2024-11-26T14:09:03Z

src/app/checkout/step-2/step-2.component.spec.js

+      })
+
+      describe('existing credit card used', () => {
+        it('should return true when cvv is invalid', () => {


You could write it as should disable continue when cvv is invalid to make the test name quicker to understand, without the dev having to look at the test itself to understand what is is testing to return true.

The same could be saids for the ones below as well.

Nice, I'll update these test names.

dr-bizz · 2024-11-26T14:12:19Z

src/app/checkout/step-2/step-2.component.spec.js

+          expect(self.controller.isContinueDisabled()).toBe(false)
+        })
+
+        it('should return false when cvv is invalid', () => {


Woudl you have a Cvv when you enter your bank details?

There's a cvv for new credit cards but not for new bank accounts. The this.selectedPaymentMethod?.['card-type’] in the isContinueDisabled conditional should ensure continue isn’t disabled for bank accounts. Line 346 should have the tests for this.

wjames111 · 2024-11-26T15:04:03Z

@dr-bizz Thanks for looking it over again! and yes, we're just taking the input on step 2, and then checking it when the user submits the checkout. The only issue with this is it never get's saved to session storage which Bill pointed out. I have a commit that should fix this issue.

wjames111 self-assigned this Oct 28, 2024

wjames111 added the On Staging Will be merged to the staging branch by Github Actions label Oct 28, 2024

wjames111 force-pushed the hideCvv-flag branch from 65c4157 to 64cfd40 Compare November 5, 2024 19:48

dr-bizz removed the On Staging Will be merged to the staging branch by Github Actions label Nov 8, 2024

wjames111 marked this pull request as draft November 8, 2024 16:05

wjames111 force-pushed the hideCvv-flag branch from 929aa54 to 24cde89 Compare November 19, 2024 20:35

wjames111 added the On Staging Will be merged to the staging branch by Github Actions label Nov 19, 2024

wjames111 marked this pull request as ready for review November 19, 2024 20:42

wjames111 requested a review from wrandall22 November 19, 2024 20:42

wrandall22 reviewed Nov 20, 2024

View reviewed changes

src/app/checkout/step-2/step-2.component.js Outdated Show resolved Hide resolved

wjames111 removed the On Staging Will be merged to the staging branch by Github Actions label Nov 20, 2024

wjames111 requested a review from wrandall22 November 20, 2024 19:26

wrandall22 requested changes Nov 20, 2024

View reviewed changes

wjames111 added 18 commits November 25, 2024 14:54

Clear securityCode viewValue on switchPayment.

ad30a26

Remove console.log.

cfbb0e4

Fix lint issues.

9e19f3a

Add check for selected payment type before disabling continue button.

db41374

Adds test for getContinueDisabled when selection is not credit-card.

3e705e0

Update to handle EFT.

d4e68b1

Add tests for new payment methods.

872c3a0

Remove hideCvv from ng-if.

ba4b235

Run linter.

375cfe9

Fix tests.

72beba9

Fixup tests.

2adbcf1

Test updates.

4ce72f3

Fix console error, refactor tests.

4fa3d1a

Fix broken test.

5d09d9d

Fix for flaky test.

4204964

Fix for needing to reenter CVV upon new payment

22c3206

cleanup tests.

aeccc63

Lint

c4717db

wjames111 force-pushed the hideCvv-flag branch from 1e72548 to c4717db Compare November 25, 2024 19:54

wjames111 requested review from dr-bizz and wrandall22 November 25, 2024 19:56

Add additional test case.

95ef487

wrandall22 reviewed Nov 25, 2024

View reviewed changes

Remove conditional, breakup test to multiple lines.

f58818e

dr-bizz reviewed Nov 26, 2024

View reviewed changes

wjames111 added 3 commits November 26, 2024 12:57

Add CVV to sessionStorage on continue; Rename tests.

0497010

lint.

8134562

Fix broken tests.

0709f6a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updates hideCvv flag for edit payment methods and new payment methods. #1109

Updates hideCvv flag for edit payment methods and new payment methods. #1109

wjames111 commented Oct 28, 2024

wjames111 commented Nov 4, 2024

wrandall22 commented Nov 5, 2024

wjames111 commented Nov 5, 2024

wrandall22 commented Nov 5, 2024

wjames111 commented Nov 5, 2024

dr-bizz commented Nov 8, 2024

dr-bizz commented Nov 8, 2024

dr-bizz commented Nov 8, 2024

wjames111 commented Nov 8, 2024

wrandall22 commented Nov 8, 2024

dr-bizz commented Nov 8, 2024 •

edited

Loading

wjames111 commented Nov 13, 2024

wrandall22 commented Nov 13, 2024

wjames111 commented Nov 14, 2024

wrandall22 commented Nov 14, 2024

wjames111 commented Nov 14, 2024

wrandall22 left a comment

wjames111 commented Nov 20, 2024

wrandall22 Nov 25, 2024

wjames111 Nov 25, 2024

wrandall22 Nov 25, 2024

dr-bizz left a comment

dr-bizz Nov 26, 2024

dr-bizz Nov 26, 2024

dr-bizz Nov 26, 2024

wjames111 Nov 26, 2024

dr-bizz Nov 26, 2024

dr-bizz Nov 26, 2024

wjames111 Nov 26, 2024

dr-bizz Nov 26, 2024

wjames111 Nov 26, 2024

wjames111 commented Nov 26, 2024

Updates hideCvv flag for edit payment methods and new payment methods. #1109

Are you sure you want to change the base?

Updates hideCvv flag for edit payment methods and new payment methods. #1109

Conversation

wjames111 commented Oct 28, 2024

wjames111 commented Nov 4, 2024

wrandall22 commented Nov 5, 2024

wjames111 commented Nov 5, 2024

wrandall22 commented Nov 5, 2024

wjames111 commented Nov 5, 2024

dr-bizz commented Nov 8, 2024

dr-bizz commented Nov 8, 2024

dr-bizz commented Nov 8, 2024

wjames111 commented Nov 8, 2024

wrandall22 commented Nov 8, 2024

dr-bizz commented Nov 8, 2024 • edited Loading

wjames111 commented Nov 13, 2024

wrandall22 commented Nov 13, 2024

wjames111 commented Nov 14, 2024

wrandall22 commented Nov 14, 2024

wjames111 commented Nov 14, 2024

wrandall22 left a comment

Choose a reason for hiding this comment

wjames111 commented Nov 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dr-bizz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wjames111 commented Nov 26, 2024

dr-bizz commented Nov 8, 2024 •

edited

Loading