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

[16.0][FIX][base_tier_validation] fix validation date timezone #810

Merged
merged 3 commits into from Feb 14, 2024

Conversation

evanomnisoft
Copy link
Contributor

Validation Date for tier reviews shows in UTC instead of user timezone.

requested_date_utc should be localized to UTC instead of user timezone

@evanomnisoft
Copy link
Contributor Author

#809

@evanomnisoft evanomnisoft marked this pull request as ready for review January 24, 2024 03:13
@evanomnisoft evanomnisoft changed the title [17.0] base_tier_validation fix validation date timezone [16.0] base_tier_validation fix validation date timezone Jan 24, 2024
@evanomnisoft evanomnisoft changed the title [16.0] base_tier_validation fix validation date timezone [16.0][FIX][base_tier_validation] fix validation date timezone Jan 24, 2024
@evanomnisoft evanomnisoft marked this pull request as draft January 24, 2024 04:12
@evanomnisoft evanomnisoft marked this pull request as ready for review January 24, 2024 04:13
@OCA-git-bot
Copy link
Contributor

Hi @LoisRForgeFlow,
some modules you are maintaining are being modified, check this out!

Copy link
Contributor

@LoisRForgeFlow LoisRForgeFlow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,

Thanks for contributing. I have a couple of questions/requests:

  • Could you provide a step by step guide to reproduce the issue you are trying to fix?
  • Also the original line you are changing is a bit weird requested_date_utc = (...utc stuff...)review.reviewed_date) I don't see how the requested date could be filled with the validation date, thoughts?

@evanomnisoft
Copy link
Contributor Author

Hi,

These are the steps to reproduce the issue

  1. Setup Odoo and User with a timezone other than UTC (eg. GMT+8)
  2. Install tier validation + another support tier validation such as sale_tier_validation
  3. Configure at least 1 tier definition
  4. Trigger the tier definition by creating a sale order
  5. Validate using the validation user set in step 2
  6. In the sale order, expand the Review section which shows the list of validators
  7. See the validation date (it will be in UTC or GMT+0 no matter what)

You are absolutely right on the variables used though. The field in question here is the datetime when the tier review has happened (field name: reviewed_date)

  1. It starts of as reviewed_date (which is essentially UTC but naive) but gets assigned to requested_date_utc then back to review_formatted_date
  2. A more accurately named alternative could be something like:
reviewed_date_utc = pytz.timezone("UTC").localize(review.reviewed_date)
            
reviewed_date_tz = reviewed_date_utc.astimezone(pytz.timezone(timezone))

review.reviewed_formated_date = reviewed_date_tz.replace(tzinfo=None)

Do you think that makes better sense?

@LoisRForgeFlow
Copy link
Contributor

@evanomnisoft Thanks for the details, I could confirm now the issue 👍

Regarding the names of the variables, yes, I would appreciate if you can do the rename. It will make the code easier to understand.

Variables were inconsistently named leading to confusion when understanding the code.
@evanomnisoft
Copy link
Contributor Author

@LoisRForgeFlow Done. Thanks!

Copy link
Contributor

@LoisRForgeFlow LoisRForgeFlow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks 👍

@LoisRForgeFlow
Copy link
Contributor

/ocabot merge patch

@OCA-git-bot
Copy link
Contributor

What a great day to merge this nice PR. Let's do it!
Prepared branch 16.0-ocabot-merge-pr-810-by-LoisRForgeFlow-bump-patch, awaiting test results.

@LoisRForgeFlow
Copy link
Contributor

@evanomnisoft could you port this to 17.0?

@OCA-git-bot OCA-git-bot merged commit 4128869 into OCA:16.0 Feb 14, 2024
5 of 6 checks passed
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at a97bd6b. Thanks a lot for contributing to OCA. ❤️

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.

None yet

4 participants