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

Browsing shipments #10249

Merged
merged 10 commits into from Mar 25, 2019
Merged

Browsing shipments #10249

merged 10 commits into from Mar 25, 2019

Conversation

AdamKasp
Copy link
Contributor

@AdamKasp AdamKasp commented Mar 20, 2019 •
edited

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Related tickets fixes #10218
License MIT

Screenshot 2019-03-25 at 09 44 40

@AdamKasp AdamKasp requested a review from a team as a code owner March 20, 2019 14:46
@Zales0123 Zales0123 added the Feature New feature proposals. label Mar 20, 2019
lchrusciel
lchrusciel approved these changes Mar 20, 2019
View reviewed changes
Copy link
Member

@lchrusciel lchrusciel left a comment

Choose a reason for hiding this comment

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

We are missing access from the menu, but it is good to go for me :)

@mamazu
Copy link
Member

mamazu commented Mar 20, 2019

You could also add a link from the shipment to the order and vice versa.

@AdamKasp AdamKasp changed the title Browsing shipments [WIP] Browsing shipments Mar 20, 2019
Zales0123
Zales0123 reviewed Mar 21, 2019
View reviewed changes
Copy link
Member

@Zales0123 Zales0123 left a comment

Choose a reason for hiding this comment

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

Generally, it's 👌 but there are lots of things that can be improved. In addition to my comments, I would add a menu entry as @lchrusciel proposed - and I see it as a must-have, as all point of this feature was to make shipments more visible at the first sight :D I would also use a label template for the shipment state as it's done, for example, on the Orders' index.

To be improved in a separate PR's:

  • "show order" button in each row
  • display of shipment total
  • "ship" button to manage shipment's directly from this index (debatable, cc @CoderMaggie)

features/shipping/managing_shipments/browsing_shipments.feature Outdated Show resolved Hide resolved
src/Sylius/Behat/Context/Ui/Admin/ManagingShipmentsContext.php Outdated Show resolved Hide resolved
src/Sylius/Behat/Context/Ui/Admin/ManagingShipmentsContext.php Outdated Show resolved Hide resolved
src/Sylius/Behat/Context/Ui/Admin/ManagingShipmentsContext.php Outdated Show resolved Hide resolved
src/Sylius/Bundle/AdminBundle/Resources/config/routing.yml Outdated Show resolved Hide resolved
@AdamKasp AdamKasp changed the title [WIP] Browsing shipments Browsing shipments Mar 21, 2019
@AdamKasp AdamKasp force-pushed the browsing-shipments branch 2 times, most recently from faaff67 to ba80a76 Compare March 25, 2019 08:00
CoderMaggie
CoderMaggie reviewed Mar 25, 2019
View reviewed changes
features/shipping/managing_shipments/browsing_shipments.feature
And the customer bought a single "Banana"
And the customer "Tony Stark" addressed it to "Rich street", "90802" "New York" in the "United States" with identical billing address
And the customer chose "UPS" shipping method with "Cash on Delivery" payment
And I am logged in as an administrator
Copy link
Member

Choose a reason for hiding this comment

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

IMO 14 lines of background is too much for browsing 2 shipments 💃

Copy link
Member

Choose a reason for hiding this comment

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

Agreed one more time, however as with this case I would just open a separate PR, focusing strictly on setup orders step refactoring. It would be useful for both this and #10233 and probably other PRs as well 🖖

@AdamKasp AdamKasp mentioned this pull request Mar 25, 2019
Merged
@Zales0123 Zales0123 merged commit 17ce675 into Sylius:master Mar 25, 2019
@Zales0123
Copy link
Member

Thanks, Adam! 🥇

Zales0123 added a commit that referenced this pull request Apr 5, 2019
@Zales0123
feature #10258 Changing shipment state in shipment index (AdamKasp)
cd8136f 
This PR was merged into the 1.5-dev branch.

Discussion
----------

| Q               | A
| --------------- | -----
| Branch?         |master
| Bug fix?        | no
| New feature?    |yes
| BC breaks?      | no
| Deprecations?   | no
| Related tickets | based on PR #10249
| License         | MIT


![Screenshot 2019-04-05 at 10 42 33](https://user-images.githubusercontent.com/29897151/55615923-f511c380-5790-11e9-8e67-e4df684e7cff.png)



Commits
-------

a7a7774 "Behats files"
daaf457 Action + routing for change shipment status
9192841 Flash message for ship
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CoderMaggie CoderMaggie CoderMaggie left review comments

@GSadee GSadee GSadee approved these changes

@Zales0123 Zales0123 Zales0123 approved these changes

@lchrusciel lchrusciel lchrusciel approved these changes

Assignees
No one assigned
Labels
Feature New feature proposals.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Admin] Shipments List
6 participants
@AdamKasp @mamazu @Zales0123 @GSadee @lchrusciel @CoderMaggie