feat: add vat number #2764

asadali145 · 2023-09-21T13:09:58Z

Pre-Flight checklist

Screenshots and design review for any changes that affect layout or styling
- Desktop screenshots
Migrations
- Migration is backwards-compatible with current production code
Testing
- Code is tested
- Changes have been manually tested

What are the relevant tickets?

(Required)
Closes #2762

What's this PR do?

(Required)
Adds a new field vat_id in the legal address model. Users can add VAT ID while creating an account and edit from the profile page. Also, Adds it to the receipts.

How should this be manually tested?

(Required)

Create a new account and verify that you can see a field toggle for VAT ID under Full Name.
Click Add VAT ID and verify that you can see the VAT ID Field.
Add some VAT ID and complete account creation.
Visit the profile page and see that you can view the VAT ID in profile page.
Click to edit the profile page. VAT ID Field should be visible in case VAT ID is added otherwise you will see a Add VAT ID toggle.
Enroll in a course and complete payment/ use Coupon.
Verify that VAT ID is added in Email Receipt and Dashboard Receipt.
Verify that VAT ID is synced with HubSpot

Screenshots

arslanashraf7

I left some feedback, Mostly questions.

arslanashraf7 · 2023-12-12T21:40:43Z

mail/templates/product_order_receipt/body.html

@@ -60,6 +60,9 @@ <h3 style="color: #000000; font-size: 16px; font-weight: 700; line-height: 18px;
                    </table>
                {% endif %}
                <strong style="font-weight: 700;">Email Address:</strong> {{ purchaser.email }}<br>
+                {% if purchaser.vat_id %}


Should we place it behind the tax display feat flag?

I don't think this is related to the taxes feature as it was an extra thing required to avoid some manual work. @cachob What are your thoughts on this?

This is a separate issue from country taxes. A learner may be required to enter a VAT number (by the employer for example) but the country itself may not require a learner's tax

arslanashraf7 · 2023-12-12T21:52:40Z

users/migrations/0015_legaladdress_vat_id.py

+        migrations.AddField(
+            model_name="legaladdress",
+            name="vat_id",
+            field=models.CharField(blank=True, max_length=255, null=True),


Is there any possibility of adding a validator on Vat ID if there is one?

I thought about that initially but there seemed to be no pattern as it varies country to country. Here is what Wikipedia says about vat ids https://en.wikipedia.org/wiki/VAT_identification_number

@cachob Do we have a specific pattern that we can validate? Is there a specific length or do we have any specific type of characters that can be used to validate a VAT ID?

As of now, Learners can add anything up to 255 characters.

let's be able to accommodate a wide array of formats and lengths. The VAT number formats and lengths may vary from country to country. However, maybe set a character limit lower than 255?

@cachob How about setting it to 50 characters?

Sounds good to me

I have added a limit of 30 characters in 52e3aeb. I saw some examples and it appeared to be at max 12-13 characters so I went for 30 to be safe.

arslanashraf7 · 2023-12-12T21:54:30Z

static/js/components/forms/ProfileFormFields.js

@@ -51,6 +51,10 @@ export const legalAddressValidation = yup.object().shape({
      .trim()
      .matches(NAME_REGEX, NAME_REGEX_FAIL_MESSAGE)
      .required(),
+    vat_id: yup
+      .string()
+      .label("VAT ID")


Should we add min/max limits to the field here?

What do you mean by min/max limits like the character limit?

Added in 52e3aeb.

arslanashraf7 · 2023-12-12T21:58:40Z

static/js/containers/pages/ReceiptPage_test.js

@@ -117,6 +118,10 @@ describe("ReceiptPage", () => {
      inner.find("#purchaserEmail").text(),
      receiptObject.purchaser.email
    )
+    assert.equal(


We should also add a test to check that the Vat ID field is hidden when there is no vat id associated with purchaser.

Added in 3fecf1f

arslanashraf7 · 2023-12-12T22:02:39Z

static/js/containers/pages/profile/EditProfilePage.js

@@ -14,12 +14,12 @@ import queries from "../../../lib/queries"
 import EditProfileForm from "../../../components/forms/EditProfileForm"

 import type { Response } from "redux-query"
-import type { Country, CurrentUser, User } from "../../../flow/authTypes"


I think we are using the EditProfile page for both the register and actual edit profile flow. In this case, I think the usage of LoggedInUser might be problematic.

EditProfilePage is only used for the user profile. RegisterDetailsPage and RegisterExtraDetailsPage are used for the registration flow. So, we can use LoggedInUser here.

arslanashraf7 · 2023-12-12T22:08:24Z

static/js/components/forms/ProfileFormFields.js

@@ -176,6 +184,23 @@ export const LegalAddressFields = ({
      />
      <ErrorMessage name="name" component={FormError} />
    </div>
+    {isVatEnabled ? (


We should write a test for this as well?

These are just the fields used in RegisterDetailsPage and EditProfilePage. We handled tests for this in the respective pages i.e. RegisterDetailsPage and EditProfilePage.

Also, we do not have any existing tests for ProfileFormFields.js. I assume all of the tests are meant to be added for the pages themselves.

asadali145 · 2023-12-26T07:54:17Z

This is ready for a re-review @arslanashraf7.

arslanashraf7

Looks good 👍 I'm approving this with a couple of questions:

Do we want to persist the VAT ID per order? - This is not happening right now. Currently, If a user changes or removed the VAT ID later on it would reflect that on the receipt page.
Add VAT button is not a toggle, Once the user has displayed the field there is no button to hide it again on the profile page. Do we want it to be a real toggle?
If the user removed the VAT ID, The label shows as empty on the profile page and hides completely. Do we want to keep these in-sync?

asadali145 · 2023-12-26T15:19:17Z

Do we want to persist the VAT ID per order? - This is not happening right now. Currently, If a user changes or removes the VAT ID later on it would reflect that on the receipt page.

Yes, there wasn't any requirement to associate it with an order. @cachob might be the best person to comment on this.

Add VAT button is not a toggle, Once the user has displayed the field there is no button to hide it again on the profile page. Do we want it to be a real toggle?

@mbilalmughal What is your opinion on this? I have implemented it as per the designs and the designs were approved by everyone.

If the user removed the VAT ID, The label shows as empty on the profile page and hides completely. Do we want to keep these in-sync?

This is done as per the requirements.

asadali145 · 2023-12-26T15:27:33Z

Merging this one. We will do any updates in follow-up PRs.

asadali145 added the Work in Progress label Sep 21, 2023

odlbot deployed to xpro-ci-pr-2764 September 21, 2023 13:10 View deployment

asadali145 added the Blocked label Oct 3, 2023

asadali145 removed the Blocked label Dec 1, 2023

asadali145 force-pushed the asad/profile-vat-number branch from 07a0af1 to 91d6c7a Compare December 1, 2023 14:04

asadali145 marked this pull request as ready for review December 4, 2023 11:18

asadali145 added Needs Review and removed Work in Progress labels Dec 4, 2023

asadali145 requested a review from arslanashraf7 December 4, 2023 11:19

arslanashraf7 self-assigned this Dec 12, 2023

arslanashraf7 requested changes Dec 13, 2023

View reviewed changes

asadali145 requested a review from arslanashraf7 December 14, 2023 09:05

asadali145 added 14 commits December 26, 2023 12:22

feat: add vat number

ea99526

updates after new designs

5d563d8

cleanup

ed97fdd

remove unused serializer field

f2cd963

fix flow errors

7f13d65

fix python tests

6d44fca

fix JS tests

12649af

add tests for RegisterDetailsForm

d726f9d

Add tests

1c4de50

add js test

f7b92b4

sync vat_id with hubspot

4284aac

fix test

d4f358b

test: add test for receipt page when no vat id

adbfb60

feat: add max limit for vat number

52e3aeb

asadali145 force-pushed the asad/profile-vat-number branch from 3fecf1f to 52e3aeb Compare December 26, 2023 07:50

arslanashraf7 approved these changes Dec 26, 2023

View reviewed changes

arslanashraf7 removed the Needs Review label Dec 26, 2023

arslanashraf7 added the Waiting on Author label Dec 26, 2023

asadali145 merged commit f3ece1e into master Dec 26, 2023
3 checks passed

odlbot mentioned this pull request Dec 27, 2023

Release 0.136.0 #2842

Merged

1 task

This was referenced Jan 4, 2024

Hubspot ApiException: (400) #2847

Closed

fix: Add vat id in hubspot properties sync #2851

Merged

arslanashraf7 mentioned this pull request Jan 29, 2024

fix: make vat_id blank only to enable profile update #2880

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add vat number #2764

feat: add vat number #2764

asadali145 commented Sep 21, 2023 •

edited

Loading

arslanashraf7 left a comment

arslanashraf7 Dec 12, 2023

asadali145 Dec 14, 2023

cachob Dec 18, 2023

arslanashraf7 Dec 12, 2023

asadali145 Dec 14, 2023

asadali145 Dec 14, 2023

cachob Dec 14, 2023

asadali145 Dec 14, 2023

cachob Dec 18, 2023

asadali145 Dec 26, 2023

arslanashraf7 Dec 12, 2023

asadali145 Dec 14, 2023

asadali145 Dec 26, 2023

arslanashraf7 Dec 12, 2023

asadali145 Dec 14, 2023

arslanashraf7 Dec 12, 2023

asadali145 Dec 14, 2023

arslanashraf7 Dec 12, 2023

asadali145 Dec 14, 2023

asadali145 Dec 14, 2023

asadali145 commented Dec 26, 2023

arslanashraf7 left a comment

asadali145 commented Dec 26, 2023

asadali145 commented Dec 26, 2023

feat: add vat number #2764

feat: add vat number #2764

Conversation

asadali145 commented Sep 21, 2023 • edited Loading

Pre-Flight checklist

What are the relevant tickets?

What's this PR do?

How should this be manually tested?

Screenshots

arslanashraf7 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

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

Choose a reason for hiding this comment

asadali145 commented Dec 26, 2023

arslanashraf7 left a comment

Choose a reason for hiding this comment

asadali145 commented Dec 26, 2023

asadali145 commented Dec 26, 2023

asadali145 commented Sep 21, 2023 •

edited

Loading