fix: membership active status check

[Fishrock123]

via @glindstr

[Fishrock123]

@glindstr the signup uses UTC times/dates, not Local. You should not need chrono_tz since we are using UTC.

See:

squamishaccess-functions/src/ipn_handler.rs

Lines 232 to 233 in 328ea1d

	let utc_now: DateTime<Utc> = Utc::now();
	let mut utc_expires: DateTime<Utc> = Utc::now() + Duration::days(365);

[glindstr]

Interesting. I suggest we use pacific time for both calculations. A member who expires on January 1st, 2025 and submits a membership check at 9:00pm pacific prevailing time on December 31st, 2024 will receive an email saying their membership has expired but an expiry date of the next day. A similar situation exists when they sign up or renew they would get an extra day at 9pm. The alternative would be to include the browser timezone in the api call but I suggest we stick with Squamish's timezone and not use utc to prevent this confusion.

The Mailchimp field for Expires and Joined is a pure date type. And it appears their system accepts and convert the RFC3339 datetime format into a pure date. I suggest we use dates instead.

Are you onboard to switch from chrono::DateTime to chrono::NaiveDate for these two fields? This would require using chrono_tz.

[Fishrock123]

If possible I would suggest converting to pacific time (or a localized timezone) when we send the email, to avoid problems with DST. This could be confusing for an admin who looks at mailchimp, I suppose, if they don't know. I think it is still better to store it in UTC in then convert it but if you feel strongly otherwise it can be changed.

[Fishrock123]

Using dates is fine but it's less standardized (because timezones), so most storage representations would still be something like RFC 3339.

[glindstr]

I agree with UTC always being best when available. In this case I believe the mailchimp system does not store UTC or any time information:

https://mailchimp.com/developer/marketing/docs/merge-fields/#structure

The field type is (naive) date and I believe when we send an RFC 3339 timestamp their system simply ignores all time/timezone information.

[Fishrock123]

I may be recollecting wrong but it may be that you still need to send an ISO date to the API.

Ok well in that case we should probably send pacific time from the ipn handler function and read it as pacific as well, and just ignore the existence of DST

[glindstr]

The code does have a comment that there was a requirement for sending datetimes. I believe my other project is sending and receiving naive Dates as strings in json. I'll double check before proceeding. I don't believe we have a DST issue. Let's see what I come back with and discuss.

[Fishrock123]

@glindstr I see the existing code in the ipn handler does something like this but it probably isn't ideal.

better would be to use DateTime::parse_from_rfc3339 (RFC 3339 is a subset of ISO 8601)

The relevant code in the ipn handler should probably also use DateTime::with_time as per the example for that, rather than how is is currently adding 365 days.

[glindstr]

I can address this. Depending on our the choice to use NaiveDate type it doesn't have a rfc3338 parsing method.

[Fishrock123]

some deprecations need to be cleaned up on the main branch...

cargo clippy --workspace --all-targets should show these

[glindstr]

I've updated the packages on the main branch and have addressed the deprecations. There are quite a few multiple versions for dependency warnings which seems fine to me but I believe the CI system on the pull request will fail them as errors.

[Fishrock123]

It looks like there are still some legitimate warnings:

 error: the borrowed expression implements the required traits
   --> src/ipn_handler.rs:213:29
    |
213 |     let hash = md5::compute(&ipn_transaction_message.payer_email.to_lowercase());
    |                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: change this to: `ipn_transaction_message.payer_email.to_lowercase()`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrows_for_generic_args
    = note: `-D clippy::needless-borrows-for-generic-args` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(clippy::needless_borrows_for_generic_args)]`

error: the borrowed expression implements the required traits
  --> src/membership_check.rs:38:29
   |
38 |     let hash = md5::compute(&email.to_lowercase());
   |                             ^^^^^^^^^^^^^^^^^^^^^ help: change this to: `email.to_lowercase()`
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrows_for_generic_args

error: could not compile `squamishaccess-functions` (lib test) due to 30 previous errors
warning: build failed, waiting for other jobs to finish...

The multiple dependency warning is hard to resolve. I suggest disabling that lint, in the following two spots:

• https://github.com/squamishaccess/squamishaccess-functions/blob/main/src/main.rs#L36
• https://github.com/squamishaccess/squamishaccess-functions/blob/main/src/lib.rs#L36

(It would be more ideal to move the lint definitions to the new Cargo.toml based config for them. Not very important but here is the documentation on that.)

[Fishrock123]

After you make that change, could you please rebase this branch?

[Fishrock123]

@glindstr good to merge whenever you like but you might want to update the code in the ipn_handler function to also use local pacific time.

For reference, I believe this is hosted in Azure's Washington datacenter but I would have to double check

[Fishrock123]

errr, you might want to fix the unused imports first though that's what failing checks.

I would recommend setting up rust-analyzer in your editor of choice.

[glindstr]

I'll update the PR for standardizing the usage of NaiveDate for your review and feedback. Thanks I'll check out rust-analyzer. I'm slowly fumbling my way through learning the rust tool set. Thank you for your patience.

[Fishrock123]

If you have a fast laptop/computer you may also want to set rust-analyzer's "check" setting to instead run clippy rather than check. That's what I suggest at my workplace but people generally have ARM MacBooks now there...

[glindstr]

I've updated the code to use NaiveDate instead of DateTime for Expiry and Joined dates. The IPN function handles live paypal transactions. I don't love making changes to working code on a critical system. Are we setup to push this to a test endpoint and use paypal test events?

[Fishrock123]

@glindstr sorry life got real over the last part of Dec

I think paypal's ipn system was always difficult to test with. I think I ran it locally and pointed their IPN simulator at it

[glindstr]

@Fishrock123 All good. Looking at this again I don't want to modify the IPN handler since it is working and vital to membership operations. I'd like to change this pull request to the membership check only and deploy the change for testing in the near future. What are you thoughts?

[Fishrock123]

If I recall correct, the IPN has a really long retry period for re-sending unprocessed events in case your handler is malfunctioning, so I don't think it is too big of an issue.

[Fishrock123]

I think the most annoying thing will be deploy, I always did it from my local windows machine. i would like to move that to GitHub actions... maybe I will have time to take a crack this weekend since I'm stuck sitting around on call ...