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

fixes #1924 #1952

Closed
wants to merge 1 commit into
base: 1-2-stable
from

Conversation

Projects
None yet
2 participants
@johanb
Contributor

johanb commented Sep 12, 2012

No description provided.

@johanb

This comment has been minimized.

Show comment
Hide comment
@johanb

johanb Sep 12, 2012

Contributor

From what I can tell from the commit history, this has actually never worked before, there has always been a mismatch between :id and :banner_id

Contributor

johanb commented Sep 12, 2012

From what I can tell from the commit history, this has actually never worked before, there has always been a mismatch between :id and :banner_id

@johanb

This comment has been minimized.

Show comment
Hide comment
@johanb

johanb Sep 13, 2012

Contributor

Commit shows the wrong issue number, it should be #1924

Contributor

johanb commented Sep 13, 2012

Commit shows the wrong issue number, it should be #1924

radar added a commit that referenced this pull request Sep 13, 2012

Fix payment methods banner non-dismissal issue #1924
Related to #1952

Conflicts:

	core/app/views/spree/admin/banners/_gateway.html.erb

radar added a commit that referenced this pull request Sep 13, 2012

@radar

This comment has been minimized.

Show comment
Hide comment
@radar

radar Sep 13, 2012

Member

Fixed with the two commits above. I like the idea of your commit. The route is defined as a member route for members and it should just pass through the banner id. It makes no sense to pass through the user in there, since it's going to be accessible in the controller.

Thanks for target painting!

Member

radar commented Sep 13, 2012

Fixed with the two commits above. I like the idea of your commit. The route is defined as a member route for members and it should just pass through the banner id. It makes no sense to pass through the user in there, since it's going to be accessible in the controller.

Thanks for target painting!

@radar radar closed this Sep 13, 2012

@johanb

This comment has been minimized.

Show comment
Hide comment
@johanb

johanb Sep 13, 2012

Contributor

hehe, you just go out of your way to prevent me from having commits in core don't you ? ;)
glad it's fixed.

Contributor

johanb commented Sep 13, 2012

hehe, you just go out of your way to prevent me from having commits in core don't you ? ;)
glad it's fixed.

@radar

This comment has been minimized.

Show comment
Hide comment
@radar

radar Sep 15, 2012

Member

Not intentionally ;)

On Thursday, 13 September 2012, Johan Bruning wrote:

hehe, you just go out of your way to prevent me from having commits in
core don't you ? ;)
glad it's fixed.


Reply to this email directly or view it on GitHubhttps://github.com/spree/spree/pull/1952#issuecomment-8528201.

Member

radar commented Sep 15, 2012

Not intentionally ;)

On Thursday, 13 September 2012, Johan Bruning wrote:

hehe, you just go out of your way to prevent me from having commits in
core don't you ? ;)
glad it's fixed.


Reply to this email directly or view it on GitHubhttps://github.com/spree/spree/pull/1952#issuecomment-8528201.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment