Skip to content
This repository has been archived by the owner on Jan 1, 2023. It is now read-only.

Default profile fields #50

Closed
OAyomide opened this issue Dec 30, 2018 · 7 comments · Fixed by #51
Closed

Default profile fields #50

OAyomide opened this issue Dec 30, 2018 · 7 comments · Fixed by #51

Comments

@OAyomide
Copy link
Contributor

According to the messenger docs, the default fields have changed and to access the other fields, one has to ask for access.

From my usage of other bots building libraries/software and also bare development without one, this will throw permission error and we'd have to change the default fields.

I am working on a simple PR to change these --the default fields.

@OAyomide
Copy link
Contributor Author

On trying a demo bot, I got this error:
(#100) Tried accessing nonexisting field (first_name) on node type (Page)

I figured its due to this issue. I think that PR should fix things

@pranas
Copy link
Contributor

pranas commented Jan 2, 2019

I experienced this problem and made a change that allows you to request the fields you want (PR #49 ). I kept the defaults so the change doesn't break applications which depends on this package. Change like this should be released with a major version bump. However, if you go ahead with changing this behaviour, I think a better approach would be to get rid of defaults and require applications to specify the fields they want. It would avoid issue like this in the future and if we change the declaration of that method and require fields to be specified it would not break applications by accident when people pull new version. /cc @paked

@paked
Copy link
Owner

paked commented Jan 2, 2019

@pranas: Historically my approach to versioning with this library has been to assume that anyone using it seriously is vendoring it themselves -- and on top of that, providing a list of breaking changes in the README to make upgrading easy.

Obviously, since we've added a go.mod file this isn't how we should do it anymore. I'm not going to lie, I forgot this yesterday! Going forward I'll attempt to be more mindful of how I handle these sorts of changes.

What you've described (removing defaults entirely) seems like the correct way to go about this, especially since it seems that Facebook have a habit of changing these defaults. From an API perspective it would be better to place the onus on developers to ensure they have the right permissions for each field they want: which I believe manually specifying them does.

Would you be able to submit a PR to add this functionality? (or maybe @OAyomide?).


In terms of versioning, once we've made that change I think we should tag a v1.0.0 release immediately so we're properly ready for Go modules. Alternatively we could tag v1.0.0 now and then another breaking change (I suppose it would be v2.0.0? afterwards...)

@pranas
Copy link
Contributor

pranas commented Jan 3, 2019

I see @OAyomide wants to contribute so he/she could do it (I could pick it up otherwise). The solution I see is changing

ProfileByID(id int64, profileFields ...string)

to

ProfileByID(id int64, profileFields []string)

and cleaning up the references to defaults. This would break the build after pulling new version and force to update applications rather than silently changing defaults and possibly breaking applications in a weird way which might even slip unnoticed.

Regarding versioning, it would be nice to tag before the change so people can lock at it and upgrade when ready.

@OAyomide
Copy link
Contributor Author

OAyomide commented Jan 3, 2019

@pranas yes! I am quite psyched about contributing as I am working on a Bot framework and this module has been extremely helpful 🙃

About the solution, Makes perfect sense to me.

I'll work on it submit the PR.

And its a He 😄

@paked
Copy link
Owner

paked commented Jan 4, 2019

RE versioning:

I've just tagged a version v0.0.1 on master.

Once #51 gets merged I will tag v1.0.0.

Thanks for your help everyone!

@paked paked closed this as completed in #51 Jan 4, 2019
@paked
Copy link
Owner

paked commented Jan 4, 2019

Done.

Let me know if you have any issues :)

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

Successfully merging a pull request may close this issue.

3 participants