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.

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

Order Cycle Management Reports #316

Closed
wants to merge 9 commits into from
Closed

Order Cycle Management Reports #316

wants to merge 9 commits into from

Conversation

lin-d-hop
Copy link
Contributor

Added a few things in this PR, expanding out the Order Cycle management Report section:

  1. For the payment methods report I added a Balance field using the User Balance Calculator
  2. I added a second report of delivery methods. This report is not yet complete, waiting on a field for frozen/chilled items.

How do I stop the merge commits (see below) from showing up on every PR? Do I actually have to cherry-pick each pull request? Seems a cumbersome way to handle this.

@lin-d-hop
Copy link
Contributor Author

ok @pmackay told me i need to rebase after a fetch upstream. So hopefully that answers the last question. In the meantime do I need to cherry-pick out those commits into a new branch? :-/

@RohanM
Copy link
Contributor

RohanM commented Jan 13, 2015

Rebasing would solve this problem. The workflow you could use might be something like this:

# Code code code
$ git fetch upstream
$ git rebase upstream/master

# Ready to create a PR
$ git push origin my-branch

# From here on, don't rebase or merge in upstream

Once you've pushed your branch to a remote server, it's bad juju to rebase it, since rebasing rewrites the history of commits, and it's very messy to reconcile them. At that point, when developing a PR, I think the best way to go is to stay on that version of master, and allow whoever's merging the PR to deal with the merge.

@RohanM
Copy link
Contributor

RohanM commented Jan 13, 2015

In the meantime, probably the best way is to cherry-pick into a new branch and open a new PR.

]
if is_payment_methods?
orders.map do |order|
ba = order.billing_address
Copy link
Contributor

Choose a reason for hiding this comment

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

This method is getting really long. That makes it harder to scan, and it's a code smell that suggests that it's time to break it up into component parts. I'd consider extracting two methods, payment_method_row(order) and delivery_row(order). Then this method can be shortened to something like:

if is_payment_methods?
  orders.map { |o| payment_method_row o }
else
  orders.map { |o| delivery_row o }
end

@lin-d-hop
Copy link
Contributor Author

Replaced by #317

@lin-d-hop lin-d-hop closed this Jan 13, 2015
@lin-d-hop lin-d-hop deleted the ordercyclemanagementreports branch January 13, 2015 10:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants