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

Refactor and add specs to stock locations helper #3827

Merged
merged 1 commit into from
Jun 28, 2022
Merged

Refactor and add specs to stock locations helper #3827

merged 1 commit into from
Jun 28, 2022

Conversation

gabrielbaldao
Copy link
Contributor

Checklist:

  • I have followed Pull Request guidelines
  • I have added a detailed description into each commit message
  • I have updated Guides and README accordingly to this change (if needed)
  • I have added tests to cover this change (if needed)
  • I have attached screenshots to this PR for visual changes (if needed)

@gabrielbaldao gabrielbaldao force-pushed the fork/refact-improve-specs-to-stocklocations-helper branch from 6f35c73 to 67717d3 Compare November 5, 2020 02:23
jarednorman
jarednorman requested changes Nov 16, 2020
View reviewed changes
Copy link
Member

@jarednorman jarednorman left a comment

Choose a reason for hiding this comment

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

This changes the behaviour when the admin_name or name are empty/blank strings rather than nil values.

Ideally the new specs should enforce this behaviour that your changes might otherwise have broken.

backend/app/helpers/spree/admin/stock_locations_helper.rb Outdated Show resolved Hide resolved
@waiting-for-dev
Copy link
Contributor

Hey @gabrielbaldao, it's been a while, but would you like to finish it with Jared's suggestion?

@waiting-for-dev waiting-for-dev force-pushed the fork/refact-improve-specs-to-stocklocations-helper branch from 02b6930 to 7ce819f Compare June 23, 2022 11:56
@waiting-for-dev
Copy link
Contributor

Hey @gabrielbaldao, it's been a while, but would you like to finish it with Jared's suggestion?

I did it myself. It should be ready for review 🙂

@waiting-for-dev waiting-for-dev requested a review from a team June 23, 2022 12:47
@gabrielbaldao
Copy link
Contributor Author

thank you @waiting-for-dev i appreciate that

spaghetticode
spaghetticode approved these changes Jun 23, 2022
View reviewed changes
Copy link
Member

@spaghetticode spaghetticode left a comment

Choose a reason for hiding this comment

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

@gabrielbaldao @waiting-for-dev thanks!

@spaghetticode spaghetticode changed the title refact and add specs to stock locations helper Refactor and add specs to stock locations helper Jun 23, 2022
aldesantis
aldesantis requested changes Jun 24, 2022
View reviewed changes
backend/spec/helpers/admin/stock_locations_helper_spec.rb Outdated
subject { helper.admin_stock_location_display_name(stock_location) }

context "without admin_name" do
let(:stock_location) { create(:stock_location_with_items) }
Copy link
Member

Choose a reason for hiding this comment

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

Can we set the name explicitly, both here and in the let statement below? Right now "NY Warehouse" is a magic string and relies on the factory's defaults—it would be good to make it explicit, for clarity and so that any changes in the factory don't affect the test.

Copy link
Contributor

Choose a reason for hiding this comment

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

It makes a lot of sense. Thanks for the suggestion. Done!

jarednorman
jarednorman reviewed Jun 24, 2022
View reviewed changes
Copy link
Member

@jarednorman jarednorman left a comment

Choose a reason for hiding this comment

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

Looks good, pending Alessandro's request to update the tests so that the value the output depends on is explicit in the setup.

@waiting-for-dev waiting-for-dev force-pushed the fork/refact-improve-specs-to-stocklocations-helper branch from 7ce819f to 6ef1800 Compare June 27, 2022 04:36
@waiting-for-dev waiting-for-dev merged commit 946dd8b into solidusio:master Jun 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@houndci-bot houndci-bot houndci-bot left review comments

@waiting-for-dev waiting-for-dev waiting-for-dev left review comments

@jarednorman jarednorman jarednorman approved these changes

@aldesantis aldesantis aldesantis approved these changes

@spaghetticode spaghetticode spaghetticode approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@gabrielbaldao @waiting-for-dev @spaghetticode @aldesantis @jarednorman @houndci-bot