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

Handle null campaigns in PATCH #3685

Merged
merged 3 commits into from
Oct 6, 2020
Merged

Handle null campaigns in PATCH #3685

merged 3 commits into from
Oct 6, 2020

Conversation

ramyaragupathy
Copy link
Member

To test, try saving a project with null campaigns and this should get through

@willemarcel
Copy link
Contributor

willemarcel commented Oct 5, 2020

@ramyaragupathy I made the query more efficient. A yet better solution would be:

        try:
            new_ids = [c.id for c in project_dto.campaigns]
            new_ids.sort()
        except TypeError:
            new_ids = []
        current_ids = [c.id for c in self.campaign]
        current_ids.sort()
        if new_ids != current_ids:
            self.campaign = Campaign.query.filter(
                Campaign.id.in_(new_ids)
            ).all()

@JorgeMartinezG some opinion on this?

@willemarcel
Copy link
Contributor

@ramyaragupathy We should apply the same solution to the Interests field

@JorgeMartinezG
Copy link
Contributor

JorgeMartinezG commented Oct 5, 2020

What about this? If you want to avoid some unneeded query when model and dto have same ids

campaigns = []
if project_dto.campaigns is not None:
    model_ids = [c.id for c in self.campaign]
    dto_ids = [c.id for c in project_dto.campaigns]

    # Get ids that are in dto but not in model (New campaigns).
    new_ids = [i for i in dto_ids if i not in model_ids]

    # Get ids that are in model but not in dto (Remove campaigns).
    removed_ids = [i for i in model_ids if i not in dto_ids]

    # If there is at least one item within the two lists, update.
    if len(new_ids) > 0 or len(removed_ids) > 0:
        campaigns = [Campaign.query.get(i) for i in dto_ids]

self.campaign = campaigns

@willemarcel
Copy link
Contributor

@JorgeMartinezG your proposal has some problems:

  • If you makes self.campaign empty on the beginning, model_ids will be empty
  • If we don't make it empty and keep that if statement, we can not make it empty by desire of the user
  • dto.campaigns can be none and raise TypeError

I added the .sort() to my proposal

@JorgeMartinezG
Copy link
Contributor

Corrected. BTW, everything is jumped when project_dto is None, not empty.

the dto has campaigns field with a default of [], so it should deserialize to an empty list if campaigns is not provided in the endpoint.

@willemarcel willemarcel added this to the 4.1.10 milestone Oct 6, 2020
@willemarcel willemarcel merged commit eedcfab into develop Oct 6, 2020
willemarcel added a commit that referenced this pull request Oct 22, 2020
* Handle null campaigns in PATCH

* make query more efficient

* only update campaigns and interests when needed

Co-authored-by: Wille Marcel <[email protected]>
@willemarcel willemarcel deleted the fix/null-campaigns branch October 22, 2020 20:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants