Move the pre-launch checklist from step 3 to 4 for the MC setup #927
Conversation
There was a problem hiding this comment.
Code looks good. Just left a few ❔ 💅 on one test file.
I was testing the PR halfway and facing a blocker in Setup MC Step 4, where the contact-information API returns null for wc_address, causing useStoreAddress to throw error "TypeError: Cannot read property 'country' of null". I have already posted this in our project slack channel (p1627923876015900-slack-C01E9LB4X61) for advice. Since the issue is not related to this PR, and this PR looks simple, I'd say feel free to merge and get ourselves unblocked.
| it( 'When all requirements are true, should pass', () => { | ||
| const values = { | ||
| website_live: true, | ||
| checkout_process_secure: true, | ||
| payment_methods_visible: true, | ||
| refund_tos_visible: true, | ||
| contact_info_visible: true, | ||
| }; | ||
|
|
||
| const errors = checkErrors( values, [], [], [] ); | ||
|
|
||
| expect( errors ).not.toHaveProperty( 'website_live' ); | ||
| expect( errors ).not.toHaveProperty( 'checkout_process_secure' ); | ||
| expect( errors ).not.toHaveProperty( 'payment_methods_visible' ); | ||
| expect( errors ).not.toHaveProperty( 'refund_tos_visible' ); | ||
| expect( errors ).not.toHaveProperty( 'contact_info_visible' ); | ||
| } ); |
There was a problem hiding this comment.
❔ I think this is similar to the other test it( 'When all checks are passed, should return an empty object',, no? I would just retain either one.
There was a problem hiding this comment.
Adding the test case of 'When all checks are passed, should return an empty object' was mentioned at #871 (comment). I think they look similar but have different purposes though. But I don't have a concrete opinion here. Probably we could have @tomalec 's thought on this?
There was a problem hiding this comment.
I agree the test is a bit redundant. What I mean in the linked comment, was mostly about the non-precise/accurate term "should pass" in the test description (which was explained in the code comment), and non 1:1 matching assertions with descriptions.
Here is an alternative suggestion, which also removes unused parameters, and snapshots:
feature/863-pre-launch-checklist...feature/863-pre-launch-checklist-tests (feel free to merge it if you like it)
PASS js/src/setup-mc/setup-stepper/store-requirements/pre-launch-checklist/checkErrors.test.js
checkErrors
Should check the presence of required properties in the given object. Returned object should have error messages for respective properties.
✓ website_live === true (5ms)
✓ checkout_process_secure === true (1ms)
✓ payment_methods_visible === true (1ms)
✓ refund_tos_visible === true
✓ contact_info_visible === true
✓ When there are many missing/invalid properties, should report them all. (1ms)
✓ When all properties are valid, should return an empty object. (1ms)
There was a problem hiding this comment.
@tomalec, thanks for the suggestion. I merged the feature/863-pre-launch-checklist-tests branch.
| it( 'should indicate multiple unpassed checks by setting properties in the returned object', () => { | ||
| const errors = checkErrors( {} ); | ||
|
|
||
| expect( errors ).toHaveProperty( 'website_live' ); | ||
| expect( errors ).toHaveProperty( 'checkout_process_secure' ); | ||
| } ); |
There was a problem hiding this comment.
❔ I think this test is not needed because it is covered by it( 'When there are any requirements are not true, should not pass'.
There was a problem hiding this comment.
Same as above #927 (comment)
There was a problem hiding this comment.
Replaced along with #927 (comment).
| // Invalid value | ||
| const values = { | ||
| website_live: false, | ||
| checkout_process_secure: 1, | ||
| payment_methods_visible: 'true', | ||
| refund_tos_visible: [], | ||
| contact_info_visible: {}, | ||
| }; |
There was a problem hiding this comment.
💅 I would split the // Not set yet and // Invalid value into two different tests, instead of within one it() block.
| expect( errors.refund_tos_visible ).toMatchSnapshot(); | ||
|
|
||
| expect( errors ).toHaveProperty( 'contact_info_visible' ); | ||
| expect( errors.contact_info_visible ).toMatchSnapshot(); |
There was a problem hiding this comment.
❔ 💅 Personally I wouldn't use toMatchSnapshot here. Let's say we change the checkErrors function, the error message is changed to a new string (e.g. 'Please check the requirements.' with an 's'), causing the snapshots to change. We would need to update the snapshots. I feel those are like frictions and the tests are not specific enough to convey those intention.
I'd say having toHaveProperty is enough, or maybe even check that the property is of string type. Whether to test for the exact string 'Please check the requirement.' is another question because currently we don't use that in the UI really, so I would leave it out for now.
There was a problem hiding this comment.
We had discussed a similar context at #893 (comment). Even though the error messages are not used on UI currently, but the context of this question is the same to me. The message texts are not the main focus here, but we still have snapshots to alert if they're changed on purpose or unintended. And it only takes an action to press a u key to update the snapshot if it's a correct change. I don't see obvious cons here.
Considering these test cases were moved out from the original checkErrors.test.js, and we had already over the deadline of this feature. I'd like to open a follow-up 📜 issue for this if this concept doesn't work for you.
Make descriptions more descriptive, remove unused parameters, remove snapshots.
Changes proposed in this Pull Request:
Partial of #863 and based on #926
js/src/setup-mc/setup-stepper/setup-free-listings/pre-launch-checklistin the folder ofjs/src/setup-mc/setup-stepper/store-requirements/Screenshots:
Kapture.2021-08-02.at.20.56.40.mp4
Detailed test instructions:
Changelog entry