{"action":"created","issue":{"url":"https://api.github.com/repos/BBGuy/commerce_stock/issues/9","repository_url":"https://api.github.com/repos/BBGuy/commerce_stock","labels_url":"https://api.github.com/repos/BBGuy/commerce_stock/issues/9/labels{/name}","comments_url":"https://api.github.com/repos/BBGuy/commerce_stock/issues/9/comments","events_url":"https://api.github.com/repos/BBGuy/commerce_stock/issues/9/events","html_url":"https://github.com/BBGuy/commerce_stock/pull/9","id":200581087,"number":9,"title":"2671372 location as entity","user":{"login":"olafkarsten","id":726968,"avatar_url":"https://avatars.githubusercontent.com/u/726968?v=3","gravatar_id":"","url":"https://api.github.com/users/olafkarsten","html_url":"https://github.com/olafkarsten","followers_url":"https://api.github.com/users/olafkarsten/followers","following_url":"https://api.github.com/users/olafkarsten/following{/other_user}","gists_url":"https://api.github.com/users/olafkarsten/gists{/gist_id}","starred_url":"https://api.github.com/users/olafkarsten/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/olafkarsten/subscriptions","organizations_url":"https://api.github.com/users/olafkarsten/orgs","repos_url":"https://api.github.com/users/olafkarsten/repos","events_url":"https://api.github.com/users/olafkarsten/events{/privacy}","received_events_url":"https://api.github.com/users/olafkarsten/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":6,"created_at":"2017-01-13T09:15:28Z","updated_at":"2017-02-24T18:37:17Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/BBGuy/commerce_stock/pulls/9","html_url":"https://github.com/BBGuy/commerce_stock/pull/9","diff_url":"https://github.com/BBGuy/commerce_stock/pull/9.diff","patch_url":"https://github.com/BBGuy/commerce_stock/pull/9.patch"},"body":"So let us begin to work on this beast.\r\n\r\nIt touches multiple issues. I will update the issues as we finish the work on this pr.\r\n\r\nFirst of all it implements the stock location entity and removes the custom implementation. Thats why we have much boilerplate stuff inbetween. In multiple places we now pass location entities instead of location_ids around. That has required multiple adjustments all over the place, too.\r\n\r\nI created an StockLocationInterface on the API Level to ensure, that we do not need the commerce stock local module for the other modules (ui to say). Probably we need to put some setters in, too. The transaction interface is not yet ready, so lets see what we need there and adjust the interface accordingly.\r\n\r\nThe PR introduces events, especially the [FILTER_STOCK_LOCATIONS](https://github.com/BBGuy/commerce_stock/compare/8.x-1.x...olafkarsten:2671372-location-as-entity?expand=1#diff-dbbc9d83eafb41a25703fc13343053fe) events. It is dispatched by the[ loadEnabled()](https://github.com/BBGuy/commerce_stock/compare/8.x-1.x...olafkarsten:2671372-location-as-entity?expand=1#diff-92eada4ee1ea7b2b588662c1bb20a13c) method of the location storage class. Test is on board for the loadEnabled() method. It returns the active locations for now. Havn't found a way to test that the event gets dispatched with the correct data. Commerce core doesn't test this. But I think with a mocked listener this should somehow work. We need to figure out Rules integration as well. We can leave both for a follow up.\r\n\r\nRemoved the getLocationsList from StockCheckInterface as we provide the locations from the StockServiceConfigInterface.\r\n\r\nRemoved the StockServiceConfig class from the API module. In my mind we don't need it. The AllwaysInStock class can return some empty arrays alone. AllwaysInStock now implements all three interfaces. One File less in the main dir. \r\n\r\nProblems:\r\n~~The [StockLevelUpdater](https://github.com/BBGuy/commerce_stock/compare/8.x-1.x...olafkarsten:2671372-location-as-entity?expand=1#diff-b4d547e0b2b49c5c5cd3aa33473e4550) is broken (cron stuff). The processItem method gets entity_id for a purchasable Entity. No idea for the moment how to efficiently load the entity. But I remember there is an issue or comment about the cron stuff.~~\r\n~~Update: Fixed it for the time beeing for products variants. There are some issues for the transaction interfaces and cron /queue handling open. Until we agree on a concept, this workes at least for product variants.~~\r\nUpdate: Fixed by another commit.\r\n\r\n~~The inline entity form throws an not so helpful error when trying to update the stock-level of the variation. Didn't find a way to debug this. No idea where it comes from.~~\r\n\r\n"},"comment":{"url":"https://api.github.com/repos/BBGuy/commerce_stock/issues/comments/282368842","html_url":"https://github.com/BBGuy/commerce_stock/pull/9#issuecomment-282368842","issue_url":"https://api.github.com/repos/BBGuy/commerce_stock/issues/9","id":282368842,"user":{"login":"olafkarsten","id":726968,"avatar_url":"https://avatars.githubusercontent.com/u/726968?v=3","gravatar_id":"","url":"https://api.github.com/users/olafkarsten","html_url":"https://github.com/olafkarsten","followers_url":"https://api.github.com/users/olafkarsten/followers","following_url":"https://api.github.com/users/olafkarsten/following{/other_user}","gists_url":"https://api.github.com/users/olafkarsten/gists{/gist_id}","starred_url":"https://api.github.com/users/olafkarsten/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/olafkarsten/subscriptions","organizations_url":"https://api.github.com/users/olafkarsten/orgs","repos_url":"https://api.github.com/users/olafkarsten/repos","events_url":"https://api.github.com/users/olafkarsten/events{/privacy}","received_events_url":"https://api.github.com/users/olafkarsten/received_events","type":"User","site_admin":false},"created_at":"2017-02-24T18:37:17Z","updated_at":"2017-02-24T18:37:17Z","body":"Hi @BBGuy \r\n\r\nThe code is 6 or 7 weeks old. We just started all kinds of refactoring all over the place. The module wasn't in the shape like it is now. At that point it seemed okay to solve multiple things in one PR. But you are right, from todays point of view it may have been better to split something away. Maybe we can figure the two things out, without the need to split it. \r\n\r\nWe have a open issue for one of your concerns. Currently we have two interfaces that defines a method getLocationsList(). StockCheckInterface and StockServiceConfigInterface. And after [a short discussion](https://www.drupal.org/node/2838526) I removed the one from StockCheckInterface.\r\n\r\nThe disappeared `getLocationList()` is more a split than a simple renaming. We need 2 slightly different functions for fetching locations. `getLocations()` returns all unfiltered Locations, e.g. for admin interfaces were you want all locations not only the active/filtered ones. `getEnabledLocations()` returns the filtered locations. Per default we filter by status and return only the active ones. Since we dispatch an event, others can further filter the list. The naming is more a convention in the commerce space. You will see getENTITY(S) all over the place. I think it's better for DX if we name our methods in contrib the same. Therefor getLocations()/getEnabledLocations() and not getLocationsList()/getEnabledLocationsList()\r\nExample: [getVariations()](https://github.com/drupalcommerce/commerce/blob/8.x-2.x/modules/product/src/Entity/ProductInterface.php#L85). \r\n\r\nAs I write this, I'm not sure whether we should do this the other way around. Maybe `getLocations()`should return the filtered locations. The second method could be `getAllLocations()`. Why? `getVariations()` (see above) returns the filtered variations too. So maybe its better we follow that.\r\n\r\nThe removal of StockServiceConfig just happens ;). I had to refactor AllwaysInStock and StockServiceConfig to fit the new locationstuff. We would have ended up with a class (StockServiceConfig) that contains [3 methods](https://github.com/BBGuy/commerce_stock/pull/9/files#diff-433d814129dddf1ed078cdfffd0aec03R52) which return emtpy arrays. I thought that does not justify a separate class. This does not effect anything outside our own AlwaysInStock.\r\n"}}
{"actor":{"display_login":"olafkarsten"}}