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

Drop Exclude Fields support and use Fields #27

Merged

Conversation

panosangelopoulos
Copy link
Collaborator

Currently, the library only supports exclude fields, but this seems extremely dangerous because whenever a new field is added to the user model, we have to remember to exclude it in the settings, it is more than likely that we forget this and the field is exposed to public by accident.

To prevent this, with the current PR we will support fields on the Meta class of the UserNode, which is read from the new setting USER_NODE_FIELDS.

The whitelist approach is much safer as using only exclude fields.

@codecov-io
Copy link

codecov-io commented Apr 13, 2020

Codecov Report

Merging #27 into master will not change coverage by %.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #27   +/-   ##
=======================================
  Coverage   97.23%   97.23%           
=======================================
  Files          12       12           
  Lines         615      615           
=======================================
  Hits          598      598           
  Misses         17       17           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 74771f4...c3dd88a. Read the comment docs.

@PedroBern
Copy link
Owner

You are totally right! But I think we should drop completely the exclude fields. I didn't understand why you revert it on the second commit. Maybe would be better to do this o v0.4, since it would drop a settings. I plan to discuss some v0.4 features on #23, I'm just not into it yet because my time is very short right now, but I'm really looking forward to it.

@panosangelopoulos
Copy link
Collaborator Author

@PedroBern thanks for approving it! I can completely drop the exclude fields but first I would like to discuss it with you. Of course, if it's ok then I can add a new commit where I will remove it.

@PedroBern
Copy link
Owner

I thank you for the PR! You know what you are doing, you can do all the necessary changes to make it work. I've just created the v0.4 branch, please make the PR for it. Also, would be great if you can update the documentation about the new changes :)

@panosangelopoulos panosangelopoulos changed the base branch from master to v0.4 April 13, 2020 17:49
@panosangelopoulos
Copy link
Collaborator Author

Sure thing, @PedroBern! Changed the base branch and update the docs accordingly.

@PedroBern PedroBern mentioned this pull request Apr 13, 2020
@PedroBern PedroBern added this to the v0.4 milestone Apr 15, 2020
@panosangelopoulos panosangelopoulos merged commit 8fa9fe3 into PedroBern:v0.4 Apr 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants