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.

Sign up for GitHub

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

Jump to bottom

[RFC] New stock management #2862

Merged
merged 7 commits into from
Oct 9, 2018
Merged

[RFC] New stock management #2862

merged 7 commits into from
Oct 9, 2018

Conversation

tvdeyen
Copy link
Member

@tvdeyen tvdeyen commented Sep 24, 2018 •
edited
Loading

The current stock management view had some flaws:

  1. You have to input absolute values. Something that is not reflecting how people actually set stock in their stores
  2. The layout was broken, because the HTML we was using is invalid
  3. The "add stock item" validation was broken, because the code was not updated after we are not using select2 anymore.

This view could definitely be improved even more, but for now I am proposing to make these small changes.

Update stock

solidus-stock-items-modify

Propagate stock to location not fulfilling the variant yet

solidus-add-stock-item

Closes #2857 #2028 #2530

@tvdeyen tvdeyen added the WIP label Sep 24, 2018
@tvdeyen tvdeyen self-assigned this Sep 24, 2018
@kennyadsl
Copy link
Member

Thanks @tvdeyen . I agree we need a UX/UI person opinion but in the meantime I personally think it's better than what we have now.

@tvdeyen tvdeyen added changelog:solidus_backend Changes to the solidus_backend gem Code review needed and removed WIP labels Sep 24, 2018
@tvdeyen tvdeyen removed their assignment Sep 24, 2018
@tvdeyen
Copy link
Member Author

tvdeyen commented Sep 24, 2018

Thanks @kennyadsl I think so as well.

@tvdeyen tvdeyen mentioned this pull request Sep 24, 2018
Closed
jacobherrington
jacobherrington approved these changes Sep 24, 2018
View reviewed changes
Copy link
Contributor

@jacobherrington jacobherrington left a comment

Choose a reason for hiding this comment

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

Tons of room for improvement on this part of the admin, but this is a huge step in the right direction!

ericsaupe
ericsaupe approved these changes Sep 26, 2018
View reviewed changes
Copy link
Contributor

@ericsaupe ericsaupe left a comment

Choose a reason for hiding this comment

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

I like this a lot!

@ccarruitero
Copy link
Contributor

@tvdeyen the changes looks good. Definitely better than the current view.

But, I'm not sure we should allow the user just update the stock. I think will be better keep a record about why the stock change, maybe generating a stock movement, that way is more easy track an error when occurred.

@tvdeyen
Copy link
Member Author

tvdeyen commented Oct 1, 2018

@ccarruitero thanks for the feedback.

But, I'm not sure we should allow the user just update the stock. I think will be better keep a record about why the stock change, maybe generating a stock movement, that way is more easy track an error when occurred.

This PR does not make any changes to our current stock API at all. It behaves exactly like before. It's only UI.

The only difference is that instead of passing the ?force=true parameter (to set an absolute stock value), it uses the default behaviour of our API that increases and decreases current stock by utilizing stock movements.

kennyadsl
kennyadsl approved these changes Oct 3, 2018
View reviewed changes
Copy link
Member

@kennyadsl kennyadsl left a comment

Choose a reason for hiding this comment

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

Thanks @tvdeyen !

tvdeyen added 7 commits October 5, 2018 09:41
@tvdeyen
Move the stock table css into one file
5d7f668 
The both stylesheets treat the same table.
@tvdeyen
Make stock management table full width
8956179
@tvdeyen
Add a meaningful blank value for stock location select
cd57851 
If no stock location is selected in the variant search forms stock
location select we display all stock locations.
@tvdeyen
Nested stock items table
0df8303 
Nest the stock location stock items into the stock management table.
Previous we misused the rowspan table attribute that led to broken
table layouts if shop is having multiple stock locations.
@tvdeyen
Modify stock count instead of setting it's absolute value
9aa35aa
@tvdeyen
Update add stock item feature
602ad18 
1. Fixes add stock item validation. Since we use native selects instead of select2 for most of our selects, this feature was broken.

2. Clarify what the purpose of add stock item select actually is

3. Update the behavior to reflect the changes we made to the edit stock item view.
@tvdeyen
Do not allow negative stock in UI
a47f881
@tvdeyen tvdeyen force-pushed the stock-items-modify branch from 38d34b5 to a47f881 Compare October 5, 2018 08:04
@tvdeyen
Copy link
Member Author

tvdeyen commented Oct 5, 2018 •
edited
Loading

I updated the PR in that you are not able to create negative stock via UI. This is prevented in the API already, but I call this better UX if we check that already in the UI.

@ericsaupe
Copy link
Contributor

Agreed, I like to have the UI handle those edge cases before a server call is made when possible. Good change!

@tvdeyen tvdeyen merged commit a79bc74 into solidusio:master Oct 9, 2018
@tvdeyen tvdeyen deleted the stock-items-modify branch October 9, 2018 07:49
This was referenced Feb 26, 2019
Closed
Closed
@kennyadsl kennyadsl mentioned this pull request Apr 2, 2019
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jacobherrington jacobherrington jacobherrington approved these changes

@ericsaupe ericsaupe ericsaupe approved these changes

@kennyadsl kennyadsl kennyadsl approved these changes

Assignees
No one assigned
Labels
changelog:solidus_backend Changes to the solidus_backend gem
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@tvdeyen @kennyadsl @ccarruitero @ericsaupe @jacobherrington