Secreto31126 / whatsapp-api-js

A TypeScript server agnostic Whatsapp's Official API framework
MIT License
128 stars 31 forks source link

Consistent Timestamp, Cross PM, typo #323

Closed ekoeryanto closed 3 months ago

ekoeryanto commented 3 months ago

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

Tests

Secreto31126 commented 3 months ago

Hey, thanks for writing this PR!

That typo was really sneaky, nice catch. On the other hand, I'm going to need a little more information with two things:

  1. I have an idea on what does the lock files should do, and I don't quite understand why you are ignoring it. Further more, I believe CI:CD will fail due to this (npm ci requires the package-lock.json file to work as expected IIRC)
  2. As far as I know, there's no such message_status property in the ServerSentMessageResponse payload. I'm testing with a v19 bot and I can't find anything similar in the logs. If you have any url to the documentation about this property, I would love to read more about it.

In other matters, I don't like the timestamp and status properties in the OnSentArgs, the first one because it might not be consistent with the API due to server timezones, and the second one because the message_status issue. I do, however, like the idea of timestamp for status and messages. Not including it for the former was an oversight from my part, and the later can access it via message.timestamp, but there's no drama with having the information duplicated for consistency within the library.

Let me know what you think about this 3 points, and we can keep working on the merge later.

Many thanks!

ekoeryanto commented 3 months ago
  1. Not every one use NPM, based on my experience, companies strict it to chosen PM they prefer. and for me personally i use only bun for PM just because there are to much cache saved if I use multiple PMs, (my laptop that only has 256 SSD forces me). If we use npm ci here, so yes maybe if will fail but it back to you then, modify it or just use the lockfile.

  2. and yes I test it too there is message_status in the response

    {
    // removed for privacy
    "response": {
    "messaging_product": "whatsapp",
    "contacts": [
      {
        "input": "628xxxxxxx",
        "wa_id": "628xxxxxxx"
      }
    ],
    "messages": [
      {
        "id": "wamid.HBgNNjI4MTIxMTEwOTg0NRUCABEYEkQ5NDgxMDQ4QUNDRjE2RTIwMAA=",
        "message_status": "accepted"
      }
    ]
    }
    }
Secreto31126 commented 3 months ago

I still don't get why the lock file would affect the end user. The package isn't released with it to npm, it's only used in development. Does the file prevents you from using bun? If possible, I would rather not change the repo's actions.


In respect to the message_status, I only found this documentation about the property:

Applies to businesses in Brazil, Colombia, and Singapore, starting September 12, 2023. Applies to all businesses starting October 12, 2023.

In theory, it should be available to everyone, but the documentation doesn't clarify if the property is always there or if it's specific for templates (which by the possible values seems to be the case).

I think the argument could be reworked into held_for_quality_assessment being a boolean, true to indicate if the property is equal to "held_for_quality_assessment", false if it's "accepted" or undefined. I believe it would create more confussion if the property is called status, as the end user might assume it can be used to check if the message was sent sucessfully, which clearly isn't its usage.


Last, if you don't mind, I would love to split this PR into 3 different ones. You can cherry pick each commit into a different branch, or if you don't know how to, I can do it for you. This is just to keep the conversations splitted, and start merging the changes that are ready, such as the typo fix.

Let me know if you agree!

ekoeryanto commented 3 months ago

I can do it for you.

great, please do it

Secreto31126 commented 3 months ago

Closing in favor of #324 #325 #326