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

Remove Pending Request Spec: Api Admin update payment state expectations. #4149

Merged
merged 1 commit into from
Sep 8, 2021
Merged

Remove Pending Request Spec: Api Admin update payment state expectations. #4149

merged 1 commit into from
Sep 8, 2021

Conversation

jcowhigjr
Copy link
Contributor

@jcowhigjr jcowhigjr commented Aug 20, 2021 •
edited by kennyadsl
Loading

Removes Pending spec: payment.reload after the request raises 'stack level too deep'

Adds expectation for payment.reload.state.
These states could lead to discussion about what the correctness of these expectations.

Description

Motivation:

As newcomer to the codebase, a pending spec is typically a decent place to start.
I could not reproduce the behavior for the skipped expectation.
I was not sure of the expected states... so added them for discussion purposes.

Checklist:

  • [ X] I have followed Pull Request guidelines
  • [ X] 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)

@jarednorman
Copy link
Member

jarednorman commented Aug 23, 2021 •
edited
Loading

This seems like a fine change as it reflects what the system currently does. Do you mind amending your commit message to follow our conventions?

@jcowhigjr
Copy link
Contributor Author

This seems like a fine change as it reflects what the system currently does. Do you mind amending your commit message to follow our conventions?

Thanks, happy to... better?

jarednorman
jarednorman approved these changes Aug 23, 2021
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.

Yep. Looks good to me.

kennyadsl
kennyadsl approved these changes Sep 1, 2021
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!

@kennyadsl
Copy link
Member

For some reason, the spec suite is not starting here. I suspect it's because you created the solidus project in your own CircleCi workspace, which prevents the main CI to run, can you please check?

@kennyadsl kennyadsl added release:major Breaking change on hold until next major release Needs Work labels Sep 1, 2021
@jcowhigjr jcowhigjr closed this Sep 2, 2021
@jcowhigjr jcowhigjr deleted the jcowhigjr-patch-1 branch September 2, 2021 21:53
@jcowhigjr jcowhigjr restored the jcowhigjr-patch-1 branch September 2, 2021 21:55
@jcowhigjr jcowhigjr reopened this Sep 2, 2021
@jcowhigjr
Copy link
Contributor Author

image
hmm ...so far I haven't found anything in my personal circleci

@kennyadsl
Copy link
Member

@jcowhigjr oh interesting, maybe it was just a temporary CircleCI issue. Can you try to force-push that branch so that it should retrigger the build, thanks in advance!

@jcowhigjr
Copy link
Contributor Author

jcowhigjr commented Sep 6, 2021 •
edited by jarednorman
Loading

image
Good idea. I added an empty commit git commit --amend --no-edit
and force pushed. https://stackoverflow.com/questions/52200096/github-pull-request-waiting-for-status-to-be-reported

Looks like it is working now.

@jarednorman
Copy link
Member

Edited your comment only to fix the image.

@jcowhigjr
Add expected payment state after admin update request
778524e 
Remove the pending/skipped spec that was previously throwing a stack level too deep error.  Add a few more expectations to describe 422 payment.states to confirm the bug is gone.
@kennyadsl kennyadsl removed Needs Work release:major Breaking change on hold until next major release labels Sep 8, 2021
@kennyadsl kennyadsl merged commit 0be17af into solidusio:master Sep 8, 2021
@jcowhigjr jcowhigjr deleted the jcowhigjr-patch-1 branch September 9, 2021 16:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kennyadsl kennyadsl kennyadsl approved these changes

@jarednorman jarednorman jarednorman 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.

None yet
3 participants
@jcowhigjr @jarednorman @kennyadsl