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

2259 refactor/api v3 label inventory #2264

Merged
merged 6 commits into from
Jun 29, 2020

Conversation

macintoshpie
Copy link
Contributor

What's this PR do?

Moves v2 /labels-taxlot and /labels-property views to v3

How should this be manually tested?

Test api/v3/labels_taxlot/ and api/v3/labels_property/ on swagger page

What are the relevant tickets?

#2259

@coveralls
Copy link

coveralls commented Jun 23, 2020

Coverage Status

Coverage decreased (-0.2%) to 71.835% when pulling 5517fc8 on 2259-refactor/api-v3-label-inventory into 4c34903 on develop.

@macintoshpie macintoshpie force-pushed the 2259-refactor/api-v3-label-inventory branch from 2d56c6c to 527f7f7 Compare June 24, 2020 20:05
@macintoshpie macintoshpie marked this pull request as ready for review June 24, 2020 20:05
@macintoshpie macintoshpie force-pushed the 2259-refactor/api-v3-label-inventory branch from 527f7f7 to ef96e77 Compare June 24, 2020 20:18
Copy link
Contributor

@adrian-lara adrian-lara left a comment

Choose a reason for hiding this comment

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

Initial review notes:

  • Perms look good!
  • Empty inventory_ids and populated add_label_ids does nothing 💯
  • Changes generally look good!

proposed changes:

  • Delete “If omitted, the effect is applied to all properties”
  • Empty inventory_ids and populated remove_label_ids does it to everything 😬
    • Maybe just error when inventory_ids are empty?

@macintoshpie
Copy link
Contributor Author

Empty inventory_ids and populated remove_label_ids does it to everything

That's appears to be the intended effect, though it should limit itself to the removal of labels that are within the org. If this is not what we want then we can change that. I'll make this PR a draft until we decide what to do here.

@macintoshpie macintoshpie marked this pull request as draft June 25, 2020 19:25
@adrian-lara
Copy link
Contributor

Empty inventory_ids and populated remove_label_ids does it to everything

That's appears to be the intended effect, though it should limit itself to the removal of labels that are within the org. If this is not what we want then we can change that. I'll make this PR a draft until we decide what to do here.

That's a good point - I could definitely imagine users wanting to just completely remove a label from their entire org's properties or tax lots.

Looking into the way it's used on the FE, inventory_ids are basically required since the Update Labels button isn't "activated" until records are checked on the inventory list page. Given that, I'd vote to keep the changes you have currently (also requiring it for the API endpoint). What do you think?

@macintoshpie macintoshpie marked this pull request as ready for review June 29, 2020 15:26
@macintoshpie
Copy link
Contributor Author

I think that sounds good 👍

Copy link
Contributor

@adrian-lara adrian-lara left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for making the final changes 🙏

@adrian-lara adrian-lara merged commit fa69bbd into develop Jun 29, 2020
@adrian-lara adrian-lara deleted the 2259-refactor/api-v3-label-inventory branch June 29, 2020 16:09
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.

3 participants