Skip to content
This repository has been archived by the owner on Jul 9, 2022. It is now read-only.

getThreadHistory: Filter out messages containing error attachments #360

Merged
merged 2 commits into from
Dec 30, 2016
Merged

getThreadHistory: Filter out messages containing error attachments #360

merged 2 commits into from
Dec 30, 2016

Conversation

ivkos
Copy link
Contributor

@ivkos ivkos commented Dec 15, 2016

Fixes #357

@Schmavery
Copy link
Owner

Thanks for taking a look at this issue. I'm not totally sure how this solves any problems, were you able to replicate the problem in #357? I had thought the exception was being thrown from formatMessage.

In an ideal world would it be nicer if we could just format these errors like we do in listen?

@ivkos
Copy link
Contributor Author

ivkos commented Dec 16, 2016 via email

@Schmavery
Copy link
Owner

Oh I see now. Thanks for clarifying!
@bsansouci what do you think, are we good to merge?

@AlexGustafsson
Copy link

Are there a way of suppressing the warnings thrown?

@ivkos
Copy link
Contributor Author

ivkos commented Dec 19, 2016

@AlexGustafsson Yeah, you can set the log level to one higher than "warn", i.e. "error" or "silent".

api.setOptions({ logLevel: "error" });

See api.setOptions for more info.

@Schmavery @bsansouci Do you think the "warn" level is appropriate for these messages? Should we use something lower, e.g. "info"?

@Schmavery
Copy link
Owner

Warn seems fine to me.
ping @bsansouci merge?

type: "error",

// Save error attachments because we're unsure of their format,
// and whether there are cases they contain something useful for debugging.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do errors really not have a specific format? Did you encounter those in the wild? If you would you mind sharing the shape of the object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bsansouci I encounter a few of them in a group chat with ~40k messages. So far, I've only come across error attachments that seem to be empty:

attachment1: { attach_type: 'error' }
attachment2: { id: '', image_data: {} }

However, I'm not sure if there are cases they contain useful information.

@bsansouci
Copy link
Collaborator

@ivkos Hey, thanks for the PR.
Am I understanding correctly that you're removing messages from the return of getThreadHistory if they had an attachment that has an error? That doesn't seem like a good behavior, as it seems like there would be no way for the user of this library to retrieve those specific message. I don't think we should filter the messages in this case.

Supporting the error type seems like a good thing though!

@Schmavery
Copy link
Owner

Thanks!

@Schmavery Schmavery merged commit 41518f8 into Schmavery:master Dec 30, 2016
how2945ard pushed a commit to how2945ard/facebook-chat-api that referenced this pull request May 30, 2017
getThreadHistory: Filter out messages containing error attachments
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants