CRM: Resolves #3303 - handle invoice status dropdown mismatches by tbradsha · Pull Request #32909 · Automattic/jetpack

tbradsha · 2023-09-06T21:15:42Z

Resolves Automattic/zero-bs-crm#3303 - handle invoice status dropdown mismatches

Proposed changes:

If the invoice status in the database doesn't match an available status, the edit page dropdown defaults to the first item on the list, making it appear to be selected.

Quotes don't have real statuses, and contact statuses are not translated. This was already solved for transactions (#30644), but we need to the same for invoices.

This PR does the following:

If the status is an unexpected one, it appends the status as a dropdown option and selects it.
WooSync was translating some statuses before saving, and now it should properly save the status in English.
Along with the above, the invoice object now has a status_label key that can be used to output the translated status (e.g. listviews, edit page, client portal, and contact/company profiles).

The larger issue involves the fact that we store labels instead of keys in the database (#1497), which hopefully will be addressed at a future date. This PR begins to lay the groundwork for invoice status keys, though the "key" stored in the database is just the English name for the status at this stage.

Note also that this won't fix any existing errant saves in the database. We haven't had any complaints on this to date, but if it comes up after this is released, the easiest fix is to do a full Woo resync.

Other information:

Have you written new tests for your changes, if applicable?
Have you checked the E2E test CI results, and verified that your changes do not break them?
Have you tested your changes on WordPress.com, if applicable (if so, you'll see a generated comment below with a script to run)?

Jetpack product discussion

Does this pull request change what data or activity we track or use?

Testing instructions:

Change the WP language to Spanish.
Create a Woo order and sync it to the CRM.

In trunk:

👍 It will show as expected in Spanish in the listview and Client Portal.
👍 It will show as expected in Spanish in the assigned contact or company profile.
👎 The status will save in the database in Spanish.
👎 Listview quickfilters don't match statuses.
👎 When editing the invoice, the status dropdown will look like it's set to the first status in the dropdown (Borrador), whereas it just wasn't able to find a match, so the dropdown defaults to the first item.
👎 If you set the invoice to a non-existent status via the database, the above point applies; the dropdown seems to have the first item selected.
👎 If you change the WP language back to English, the status shows in Spanish for the first two points.

In the fix/crm/3303-fix_invoice_status_dropdown_mismatches branch:

👍 It will show as expected in Spanish in the listview and Client Portal.
👍 It will show as expected in Spanish in the assigned contact or company profile.
👍 The status will save in the database in English.
👍 Listview quickfilters match statuses.
👍 When editing the invoice, the status dropdown will properly select the current status.
👍 If you set the invoice to a non-existent status via the database, the dropdown shows said status as selected.
👍 If you change the WP language back to English, the status shows in English everywhere.

github-actions · 2023-09-06T21:17:01Z

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

✅ Include a description of your PR changes.
✅ Add a "[Status]" label (In Progress, Needs Team Review, ...).
✅ Add testing instructions.
✅ Specify whether this PR includes any changes to data or privacy.
✅ Add changelog entries to affected projects

This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖

The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available.

Once your PR is ready for review, check one last time that all required checks (other than "Required review") appearing at the bottom of this PR are passing or skipped.
Then, add the "[Status] Needs Team Review" label and ask someone from your team review the code. Once reviewed, it can then be merged.
If you need an extra review from someone familiar with the codebase, you can update the labels from "[Status] Needs Team Review" to "[Status] Needs Review", and in that case Jetpack Approvers will do a final review of your PR.

Crm plugin:

Next scheduled release: September 27, 2023.
Scheduled code freeze: September 20, 2023.

gogdzl · 2023-09-08T18:02:25Z

projects/plugins/crm/modules/woo-sync/includes/class-woo-sync.php

 			if ( in_array( $order_status, $paid_statuses, true ) ) {
-				$status = __( 'Paid', 'zero-bs-crm' );
+				$status = 'Paid';
 			} elseif ( $order_status === 'checkout-draft' ) {
-				$status = __( 'Draft', 'zero-bs-crm' );
+				$status = 'Draft';
 			} else {
-				$status = __( 'Unpaid', 'zero-bs-crm' );
+				$status = 'Unpaid';
 			}


Don't we need to add some kind of migration to be backwards compatible?

We have this problem throughout the whole codebase.
E.g.:

includes/ZeroBSCRM.DAL3.Helpers.php // retrieves statuses from object :) function zeroBSCRM_getInvoicesStatuses(){ // for DAL3+ these are hard typed, probably need to sit in the obj: return array( __( 'Draft', 'zero-bs-crm' ), __( 'Unpaid', 'zero-bs-crm' ), __( 'Paid', 'zero-bs-crm' ), __( 'Overdue', 'zero-bs-crm' ), __( 'Deleted', 'zero-bs-crm' ) ); }

Don't we need to add some kind of migration to be backwards-compatible?

I mentioned this briefly above; to my knowledge, only WooSync had this bug. A migration gets messy because we have know way of knowing if:

The user has changed languages since, or

The value in the database is not something they manually added.

Since there's an easy workaround (Woo resync), I think avoiding a migration is best until we actually move to some real key-based status setup.

We have this problem throughout the whole codebase.

Indeed, my goal here was primarily to address the specific Woo situation, and to make output more i18n-friendly. But I'll poke around and see if there are other cases we can clean things up.

Fixed code block that was meant to set an invoice to "Unpaid" if it was "Draft" and emailed to a user: 56380a3

Because 'Draft' !== __( 'Draft', 'zero-bs-crm' ) if the status has translations, the condition was never met previously.

Fixed invoice bulk action status change which hasn't worked previously on non-English installs: ab85f34

WooSync status mapping was previously using translated status, but now correctly uses the English status with translated output: 6ebf0c0

tbradsha · 2023-09-08T20:24:47Z

@gogdzl I've widened the scope of this PR to cover all invoice statuses I could find, and I think they're all properly handled (along with some bug fixes that have existed since who knows when).

tbradsha · 2023-09-15T12:43:13Z

Rebased, as this has been sitting some time...

gogdzl

It works and it is a solid move towards fixing our statuses issues with translation.

Great job!

cleacos · 2023-09-21T10:18:17Z

A minor issue with this change. This happens on the WooSync order mapping in sites translated to other languages:

Before the change:

After the change:

As you can see, any previous mapping will be reset to the default. So if the user had a different mapping (low probability), they will lose it.

tbradsha · 2023-09-21T13:03:03Z

Thanks, @cleacos! I have notes from our call as well and will look into this today to see if it's something we can/should address.

tbradsha · 2023-09-21T19:34:12Z

Handled in #33254.

tbradsha added [Status] Needs Team Review Obsolete. Use Needs Review instead. [Plugin] CRM Issues about the Jetpack CRM plugin labels Sep 6, 2023

tbradsha requested a review from a team September 6, 2023 21:15

tbradsha self-assigned this Sep 6, 2023

github-actions bot added [CRM] Woo Sync Module Admin Page React-powered dashboard under the Jetpack menu labels Sep 6, 2023

gogdzl reviewed Sep 8, 2023

View reviewed changes

github-actions bot added the [CRM] Portal Module label Sep 8, 2023

tbradsha requested a review from gogdzl September 8, 2023 20:24

tbradsha added 13 commits September 15, 2023 06:42

Add dropdown option if current status is non-standard

99f1ea8

Save untranslated invoice status in database

b9eafc8

Add translated status label to invoice object

08b29f0

Use translated status label when appropriate

407574f

Add changelog

9d4d21b

Set invoice to UnPaid if Draft when emailed

a49b980

Fix invoice bulk action status change

58596db

Fixed Woo mapping to output translated status

d7f3c4d

Code cleanup

e2bc1a2

Translated → Untranslated

f900905

Compare untranslated strings

9d84379

Cleanup

dbb062a

Fix typo

e7c36dc

tbradsha force-pushed the fix/crm/3303-fix_invoice_status_dropdown_mismatches branch from 5c2b7b2 to e7c36dc Compare September 15, 2023 12:43

tbradsha requested a review from a team September 15, 2023 12:47

gogdzl approved these changes Sep 15, 2023

View reviewed changes

tbradsha merged commit 393dc93 into trunk Sep 15, 2023

tbradsha deleted the fix/crm/3303-fix_invoice_status_dropdown_mismatches branch September 15, 2023 13:26

github-actions bot added this to the crm/6.1.1 milestone Sep 15, 2023

github-actions bot removed the [Status] Needs Team Review Obsolete. Use Needs Review instead. label Sep 15, 2023

tbradsha mentioned this pull request Sep 19, 2023

CRM: Automations - add the invoice entity class #33137

Merged

3 tasks

tbradsha mentioned this pull request Sep 21, 2023

CRM: Resolves #3337 - add invoice status and mapping settings migration #33254

Merged

3 tasks

tbradsha added [CRM] WooSync Module and removed [CRM] Woo Sync Module labels Oct 20, 2023

Conversation

tbradsha commented Sep 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed changes:

Other information:

Jetpack product discussion

Does this pull request change what data or activity we track or use?

Testing instructions:

Uh oh!

github-actions bot commented Sep 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gogdzl Sep 8, 2023

Choose a reason for hiding this comment

Uh oh!

gogdzl Sep 8, 2023

Choose a reason for hiding this comment

Uh oh!

tbradsha Sep 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tbradsha Sep 8, 2023

Choose a reason for hiding this comment

Uh oh!

tbradsha Sep 8, 2023

Choose a reason for hiding this comment

Uh oh!

tbradsha Sep 8, 2023

Choose a reason for hiding this comment

Uh oh!

tbradsha Sep 8, 2023

Choose a reason for hiding this comment

Uh oh!

tbradsha commented Sep 8, 2023

Uh oh!

tbradsha commented Sep 15, 2023

Uh oh!

gogdzl left a comment

Choose a reason for hiding this comment

Uh oh!

cleacos commented Sep 21, 2023

Uh oh!

tbradsha commented Sep 21, 2023

Uh oh!

tbradsha commented Sep 21, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tbradsha commented Sep 6, 2023 •

edited

Loading

github-actions bot commented Sep 6, 2023 •

edited

Loading

tbradsha Sep 8, 2023 •

edited

Loading