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

Revert "Merge pull request #2893 from luisramos0/deleted_products_bre… #2961

Closed
wants to merge 1 commit into from

Conversation

sauloperez
Copy link
Contributor

…ak_inventory"

What? Why?

This reverts commit 8a3f621, reversing
changes made to 7cac463.

…leted_products_break_inventory"

This reverts commit 8a3f621, reversing
changes made to 7cac463.
@sauloperez
Copy link
Contributor Author

@luisramos0 feel free to share a link to further details when you have them.

@daniellemoorhead
Copy link
Contributor

@sauloperez #assignyourself #please 🙏

@mkllnk
Copy link
Member

mkllnk commented Oct 31, 2018

@luisramos0 I just read through the thread at #2893 and there are still two open questions:

  1. Why are permissions in place if the user sells their own enterprise? Is this scenario still valid?
  2. Does reverting fix anything? If the data changed because of the migration, this pull request doesn't change anything about it. The migration didn't have a down method and there is no way for a rollback via migration. The user already fixed the stock levels herself. So maybe no further code change is needed for Australia.

@luisramos0
Copy link
Contributor

Maikel, good questions:
1- both the code and the migration will revoke inventory items even if override hub_id = produdct supplier_id. we need to fix this in both code and migration.
2- In AUS now, everytime users create permissions, the data will be corrupted again. We should run quick fix (see below) to fix AUS data and revert the change. the more you keep the system as is the more inventories will be messed up.

See here:
#2960 (comment)

@luisramos0
Copy link
Contributor

Actually, changing my mind.
@mkllnk is right on point 2. This code is only revoking permissions on delete (user needs to delete permissions) and will only affect the overrides of that permission (specific relation between 1 producer and 1 hub), no own overrides involved. Also, the code will review overrides when new permission is created but again it will not affect own overrides.

So, the bug is only in the migration. We need to fix it and run it again.

@luisramos0 luisramos0 closed this Oct 31, 2018
Copy link
Contributor

@luisramos0 luisramos0 left a comment

Choose a reason for hiding this comment

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

Thanks @sauloperez this is not needed after all. Apologies!

@luisramos0
Copy link
Contributor

And, correcting what I say above about point 1.
1- Only the migration will revoke inventory items even if override hub_id = product supplier_id.
The code will only revoke permissions that are part of the permission being created or deleted.

So, actually there is a problem in the code when a permission to manage own inventory is added and then removed (it's a edge case, but will break the inventory). the fix is to re-add the permission. I think we can ignore this case but I'll put this on 2893 for reference.

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.

4 participants