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

Apply User-Defined Date Formatting Settings to WP Admin React Components #9635

Open
wants to merge 103 commits into
base: develop
Choose a base branch
from

Conversation

mgascam
Copy link
Contributor

@mgascam mgascam commented Oct 28, 2024

Fixes #6567

This pull request includes changes to ensure that user-defined date and time formatting settings are respected across various React components in the WP Admin UI. The most important changes involve replacing the dateI18n function with custom date formatting functions and adding a new component to inform users about the update.

Changes proposed in this Pull Request

Date and Time Formatting Updates:

New Component Addition:

Test Updates:

  • Updated various test files to include dateFormat in the global settings to ensure tests respect the user-defined date format. [1] [2] [3] [4] [5] [6] [7] [8] [9]

Testing instructions

General Date and Time Display Testing
  • Navigate to WP Admin > Settings > General and set your preferred Date and Time Formats and Timezone.
  • Verify that the date format matches the user-defined settings in WordPress when you navigate to the WooPayments plugin pages.
  • Verify that the dates and times are correctly converted from UTC to your preferred timezone.
Component Specific Testing

Transactions

  • Navigate to WP Admin > Payments > Transactions.
  • Verify the values in Date / Time and Deposit Date columns.
Screenshot 2024-10-29 at 17 12 32 Screenshot 2024-10-29 at 17 12 37

Capital Loans

This section is hidden if your user does not have capital loans. Follow the steps in this guide to create one offer for your user. Once the offer is created, your user will get an email to confirm it. Visit the link in the email, but make sure to use your local store URL, not the server one.

  • Navigate to WP Admin > Payments > Capital loans
  • Verify that the Active loan overview dates are formatted according to the user settings.
  • Verify that the All Loans table dates are formatted according to the user settings. There are two columns that contain dates: Disbursed and First Paydown.
Screenshot 2024-11-04 at 12 17 37

Deposits

  • Navigate to WP Admin > Payments > Deposits
  • Verify that the dates in the Deposit history table are formatted according to the user settings
Screenshot 2024-11-04 at 12 23 38
  • Click on a row and verify that the date and times in the Date /Time column of the deposit details are formatted correctly.
Screenshot 2024-11-04 at 12 24 13

Disputes
You can use test cards to create disputed orders.

  • Navigate to WP Admin > Payments > Disputes
  • Verify the date and time in the Disputed on column (you may need to reveal it)
Screenshot 2024-11-04 at 12 40 28 - Click one row to navigate to the details view and verify the dates Screenshot 2024-11-04 at 12 41 16
  • Click on Challenge Dispute and verify the dates in the form are correct
Screenshot 2024-11-04 at 12 45 21

Documents

In the server, you can use the cli to add a document to your account wp wcpay add_document_for_account 236870460 vat_invoice --period_from="2024-10-01 00:00:00" --period_to="2024-10-31 23:59:59"

  • Navigate to WP Admin > Payments > Documents
  • Verify that the dates in the Date and Description columns in the Documents list are correctly formatted
Screenshot 2024-11-04 at 13 07 17
  • Run npm run changelog to add a changelog file, choose patch to leave it empty if the change is not significant. You can add multiple changelog files in one PR by running this command a few times.
  • Covered with tests (or have a good reason not to test in description ☝️)
  • Tested on mobile (or does not apply)

Post merge

@rogermattic
Copy link

rogermattic commented Dec 4, 2024

If this placement isn’t acceptable, we’ll need to add a new feature to the TableCard component in the WooCommerce repo and wait for its release before proceeding. While I’d prefer to ship this sooner, I’ll follow your lead here.

I mean I'd prefer the placement within the table but if this increases the complexity much, I'd understand this.

With regards to the Payments Overview though, are we placing it there cause the change also reflects in the mini Deposits overview table? I personally wouldn't advise we have it there otherwise, and even in that case... I'm not sure this needs to be surfaced there... rather than only in the actual Deposits table on the Deposits page. WDYT?

I implemented a simple dismiss functionality using browser local storage to persist the user’s preference. This means the notice will remain dismissed unless the user clears their browser’s local storage.

Does it mean that if the user clears the cache, the previously dismissed banner comes back? I'm asking just out of curiosity, as I think this isn't a big deal. And like you say... we should probably keep it for a few release cycles only.

@rogermattic
Copy link

Actually, after a bit more thinking... I understand it iseasier to implement it above the table, and I hate being stubborn and saying this but I’m leaning towards suggesting that we implement it within the table instead. Unless this really increases the effort massively??
The inline notice doesn’t work well on the gray background, and the alternative general notice style feels too disconnected from the context. A

@mgascam mgascam added the status: blocked The issue is blocked from progressing, waiting for another piece of work to be done. label Dec 10, 2024
@mgascam
Copy link
Contributor Author

mgascam commented Dec 10, 2024

Added blocked status until we implement the inline notification in the TableCard header: woocommerce/woocommerce#53496. Please check previous comments for context.

@haszari
Copy link
Contributor

haszari commented Dec 12, 2024

client/components/date-format-notice/index.tsx: Added a new DateFormatNotice component to inform users about the new date and time formatting settings and provide a link to configure these settings.

Added blocked status until we implement the inline notification in the TableCard header: woocommerce/woocommerce#53496. Please check previous #9635 (comment) for context.

This is an important PR – making dates respect user preferences. Here's my understanding of why it's blocked:

  • We're changing the date formatting.
  • So we want to tell the merchant that things changed.
  • The mechanism / UX we want to use for that notice is not implemented and needs a change in WooCommerce core.

This is unfortunate – we should be able to fix date formatting without implementing a new kind of notice.

Here are some options I think are reasonable:

  1. Ship the change with a release note and clarify in docs (and any release comms). I.e. quietly change it.
  2. Ship the change with a release note, clarify in docs etc as above AND implement some kind of notice in the admin UI. Any kind of notice, whatever works best / we can ship without a hassle.

@rogermattic @mgascam would you be concerned if we went with (1) approach – ship quietly? Do we expect significant support requests – @AashikP keen for your thoughts on risk of shipping quietly.

@rogermattic – how important is it to you that we delay shipping the date fix until we have a new kind of notice implemented in core?

After considering the importance of this announcement and how noticeable it should be, I decided to use an inline notice. It’s important enough to stand out but doesn’t need the prominence of something like a spotlight, which might be too distracting.

Personally, I think a spotlight notice is reasonable. It's a little loud, but this is a pervasive change (respecting date format throughout WooPayments), so a "global" notice is a good fit. We used a spotlight for payouts which was a less impactful and more localised change.

@rogermattic
Copy link

rogermattic commented Dec 12, 2024

Here are some options I think are reasonable:

  1. Ship the change with a release note and clarify in docs (and any release comms). I.e. quietly change it.
  2. Ship the change with a release note, clarify in docs etc as above AND implement some kind of notice in the admin UI. Any kind of notice, whatever works best / we can ship without a hassle.

@rogermattic @mgascam would you be concerned if we went with (1) approach – ship quietly? Do we expect significant support requests – @AashikP keen for your thoughts on risk of shipping quietly.

I’d definitely go for option 2. I think it could get confusing for users if they see a change and don’t know how to get back to their previous setup.

  1. They might not realise how their preferences are currently set up.
  2. They could worry about exporting, especially if they have something configured elsewhere that depends on the date column.

So yeah, I think it’s quite important to show some kind of context or information.

@rogermattic – how important is it to you that we delay shipping the date fix until we have a new kind of notice implemented in core?

At first, I thought we already had that inline notice ready, so I definitely didn’t mean to delay shipping. I only suggested it because I thought it’d be helpful to have the notice closer to the context —that’s all. Sorry for complicating @mgascam ’s life in the process!

This is an important PR – making dates respect user preferences.

@mgascam apologies, but would it still be possible to somehow go back to the notice style and positioning that you had implemented initially, just with the copy I proposed?

image

@AashikP
Copy link

AashikP commented Dec 13, 2024

@AashikP keen for your thoughts on risk of shipping quietly.

From what I understand, this is a very welcome change that implements consistency across WooPayments. Is there a specific reason we'd want to ship this quietly?

Unless we do, there should be some (any) form of notice to the merchants. Without that, we can expect some support requests. Maybe it is because of confusion and, in some cases, anger because they never noticed that the time now reflects the time they've configured under General settings.

One extreme example is when the merchant blames us for the lost dispute. They might say they missed the deadline because they assumed the time was in UTC, their time zone, etc.

@aheckler We should also add a notice in the public docs letting them know what timezone WooPayments uses and how they can change it.

cc @francoishvz

@aheckler
Copy link
Member

IMHO we should just ship this and not worry about it. No announcement needed other than the release notes / changelog.

The fact that the dates and times shown by WooPayments did NOT respect the merchant's core WP settings from the very beginning is/was a bug. This PR fixes that bug. And we release bug fixes all the time with zero notice. I don't think this one needs to be any different.

I wouldn't even bother mentioning this in the docs to be honest, since following the core WP settings is the obviously correct thing to do.

@AashikP
Copy link

AashikP commented Dec 13, 2024

The fact that the dates and times shown by WooPayments did NOT respect the merchant's core WP settings from the very beginning is/was a bug.

@aheckler Now that you mention it, you're right! I totally missed that this was originally reported as a bug. I assumed this to be a feature update.

There's only one merchant report in the bug report, and we will get back to them during the release phase.

We don't usually provide notices for bug fixes. That said, I'd still consider this notice as nice to have just to avoid confusion.

The public docs update is understandable since this was a bug to begin with. We don't need to specifically mention this in the docs.

@mgascam mgascam removed the status: blocked The issue is blocked from progressing, waiting for another piece of work to be done. label Dec 13, 2024
@mgascam
Copy link
Contributor Author

mgascam commented Dec 13, 2024

Thank you, everyone, for your helpful comments!

Ship the change with a release note, clarify in docs etc as above AND implement some kind of notice in the admin UI. Any kind of notice, whatever works best / we can ship without a hassle.

@haszari I also prefer this option, and it seems we are all aligned on it. I've made the necessary changes to use the BannerNotice component in this commit.

For context, I completely agree that this is a bug. However, my main concern has been avoiding unintended negative impacts from fixing it, especially given the low number of complaints. The suggestion to show a notice for merchants resonated with me as a practical solution. Additionally, I appreciated @rogermattic's idea to place the notice within the table context, as it would have been more targeted.

Initially, I assumed it would be straightforward to implement this change using the new TableCard component from the woocommerce/components repository. However, after gaining more experience, I realized I underestimated the complexity. Specifically, the plugin relies on the woocommerce/components bundle from WooCommerce core, which means we would need to wait for a new WooCommerce core release. This approach would be problematic, as merchants who update WooPayments without updating WooCommerce core would not see the notice.

Given this limitation, I believe moving forward with the BannerNotice is the best option. While not ideal, it ensures we can inform merchants about the change effectively.

I've added the notice to all pages under the Payments navigation menu. Below are some screenshots for your review—please let me know your thoughts.

Lastly, I’ll be on extended AFK starting Dec 23rd, so it would be great to get additional reviews and feedback next week. Thank you!

Screenshot 2024-12-13 at 17 38 52 Screenshot 2024-12-13 at 17 39 09 Screenshot 2024-12-13 at 17 38 52

cc: @AashikP @aheckler

@AashikP
Copy link

AashikP commented Dec 14, 2024

Looks good to me! Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

User set Date and Time formatting aren't respected in react components
9 participants