Create an admin screen this should be using a similar url structure to the 7 stock with a default overview tab and this screen as a tab.

We need to be able to:
* Create stock locations.
* Edit the location name.
* Delete ?? if no transaction - this probably needs thinking about so maybe for a later stage.

This does not need to be a part of the API so no need to update the interfaces and the storage API class

Comments

guy_schneerson created an issue. See original summary.

steveoliver’s picture

steveoliver commented
Assigned: Unassigned » steveoliver
guy_schneerson’s picture

guy_schneerson commented

Hi @steveoliver that will be a great help. If you got the time would be good sorting out the menu routs and menu entries for the two other pages alongside this one:

  • commerce_stock.admin_settings path: '/admin/commerce/config/stock'
  • stock_ui.stock_transactions1path: '/admin/commerce/config/stock/transactions1'

Notes: the transactions1 page is the first part of a two step form. step one chose a product. step two create a transaction. Step 2 can now also be called directly from the stock edit widget.

steveoliver’s picture

steveoliver commented
Parent issue: » #2834966: [META] Commerce_stock release 8.x-beta1
guy_schneerson’s picture

guy_schneerson commented
Related issues: +#2838050: Add further possibilities to filter stock locations available for stock allocation

I have create a related issue #2838050: Add further possibilities to filter stock locations available for stock allocation

olafkarsten’s picture

olafkarsten commented

Should we not implement a more sophisticated location solution, a content entity type to be more clear?

I would bet one will soon need a fieldable thing to add adresses or some other stuff. A Drupal entity gives us much of the ui stuff and all the handling for free. I we do it with bundles, we can even have location types (car, store, warehouse) if one need it. Though commerce_stock should only provide a default one.

guy_schneerson’s picture

guy_schneerson commented

Hi @olafkarsten I agree an entity would make sense and I know people would like to create whorehouses as locations. Got three comments:
1) Not high priority in my mind, but useful just the same.
2) Can we archive the same thing with a reference field to the schema table? ()
3) If we go with entities we need to make sure this does not impact on performance. currently queries are light and officiant. This should only involve getLocationList() and we can cache the result of this function.

olafkarsten’s picture

olafkarsten commented

1) As an feature is it not prio A, I agree. But I guess it is easier and faster to create the entity, get all the ui and drupal integration for free, than implement an own solution.

2) +3) I'm not a database guru. Do you have any actual number/experience in drupal db performance? I would expect, the drupal core entity system, db-layer and caching stuff is highly omptimized. Especially in cases like "locations", that won't change that often. I would think any own solution will end in lower performance.

And last but not least - multilingual. Own solution means, you have only single language implementation or have to implement you own translation stuff.

steveoliver’s picture

steveoliver commented
Assigned: steveoliver » Unassigned

I know olaf is working on this, and he has shown that loading entities is going to be just as performant as hitting the db directly. I think the benefits of locations as content entities cannot be overstated.

guy_schneerson’s picture

guy_schneerson commented

Thanks both, I am conviced Entity are the way to go. It will be impossible for entities to perform better then the code we currently have as it is a single select, however it is unlikely to be an issue and as I was saying the only place it is currently used is getLocationList() and we can add caching to the function if needed.

steveoliver’s picture

steveoliver commented

Olaf is working on the location as entity approach at https://github.com/olafkarsten/commerce_stock/commits/2671372-location

olafkarsten’s picture

olafkarsten commented
Assigned: Unassigned » olafkarsten

lets make it official ;)

steveoliver’s picture

steveoliver commented
Title: Create interface for managing locations » Manage stock locations as entities
steveoliver’s picture

steveoliver commented
Parent issue: #2834966: [META] Commerce_stock release 8.x-beta1 » #2838050: Add further possibilities to filter stock locations available for stock allocation
Related issues: -#2838050: Add further possibilities to filter stock locations available for stock allocation
steveoliver’s picture

steveoliver commented
Status: Active » Needs review

Nice work, @olaf. Just needs a rebase and I think it looks good to me. @BBGuy this is ready for your review -- https://github.com/BBGuy/commerce_stock/pull/9

guy_schneerson’s picture

guy_schneerson commented

Great work.
Mostly happy. My only worry is that the PR is making some changes that don't look to me like directly related to this task, see my comment on github If that is the case we should separate those changes to another issue.

olafkarsten’s picture

olafkarsten commented

New PR with all things removed, not essentially related to that issue
https://github.com/BBGuy/commerce_stock/pull/25

guy_schneerson’s picture

guy_schneerson commented
Assigned: olafkarsten » Unassigned
Status: Needs review » Fixed

Thanks @olafkarsten great work.
I have separated some of the code cleanup changes that was not directly related to this task and will push in another commit.

steveoliver’s picture

steveoliver commented
Status: Fixed » Needs work

Unfortunately this is not done yet -- see https://github.com/BBGuy/commerce_stock/pull/25#issuecomment-286162743. Also, we should commit only after we have a passing build. :/

steveoliver’s picture

steveoliver commented
Status: Needs work » Fixed

OK, I spoke too soon! Technically, this works (on a fresh install). But there are a few cleanups and an upgrade path (from previous -dev versions) I'll propose in new child issues.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.