Skip to content

Add tests for every scenario of the relationship operations #47

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
Merged
merged 7 commits into from
Feb 21, 2017
Merged

Add tests for every scenario of the relationship operations #47

merged 7 commits into from
Feb 21, 2017

Conversation

@valscion
Copy link
Member

@valscion valscion commented Feb 20, 2017

We want to make sure the tests for the new cases match all the possible cases:

  • authorized with finer grained policy but NOT authorized with update?
  • NOT authorized with finer grained policy but authorized with update?
  • authorized with both finer grained policy and update?
  • ☝️ This also means that the current tests could be merged together

I needed to do some refactoring with the stubbing logic to make it work correctly for all these cases.

@valscion
Swap order of PunditStubs arguments
57e69da
@valscion
Stub multiple policy methods at same time
d258bfd 
Without this, new allow_action/disallow_action calls for the same record
would accidentally override and disable the previous stubs
@valscion
Add more detail to test names of cases stubbing multiple policy actions
df05c07 
This allows us to see that there might be some cases untested. Now the
test names show that there might be a matrix of different cases that
should be tested
@valscion
Consolidate order of multiple actions tubs in test names
97163af
@valscion
Add explicit tests for one-sided authrozation policy methods
8bbae32 
These tests seemed to be missing in the original pull request. This
commit is what all the other previous commits for spec changes and
stubbing changes were leading up to
@valscion
Squish stub_policy_actions calls to one-line before blocks
3ad54ec
@valscion
Merge relationship authorization positive specs
5e51e82 
When both methods aren't stubbed, the mocking is unhelpfully bypassed
and the policy acts like the other method wouldn't be present at all.
@valscion valscion force-pushed the test-updates-for-authorize-relationships branch from fc64038 to 5e51e82 Compare February 20, 2017 08:26
@valscion
Copy link
Member Author

valscion commented Feb 20, 2017

Review from @lime or @matthias-g would be appreciated ☺️

I added you as a collaborator now, @matthias-g 🎉. All pull requests will need a review before merging, so if you want to help you can review open pull requests, too, and give comments to help the PRs advance.

You'll notice that we now also have a few labels — these are similar to what we use internally at Venuu, so let me walk through them quickly:

Type labels

These labels allow us to distinguish issues and PRs from each other. It's OK if a PR or an issue doesn't have any type label set.

Type: Discussion

  • Applied to issues that are actually discussions

Type: Bug

  • Applied to issues that are bugs (something is broken) and PRs that fix bugs.

Type: Critical bug

  • Applied to issues and PRs that fix severe bugs.
  • This label should not be added lightly — it is something like "THE SHIP IS ON FIRE" and should only be used if something very severe is wrong.

Status labels

These are labels that would be set by the PR author, if it would be possible. They allow us to see when the PR is in need of a review or other help.

Help: Input appreciated

  • Someone has trouble continuing with a PR and needs outside comments to continue.
  • After the question is resolved and work can continue, this label should be removed.
  • We try to answer cases with this label quickly

Status: Ready for review

  • A pull request is ready for reviewing and waits for a collaborator to review it

Status: PR / Help wanted

  • Applied to issues that would accept a PR closing the issue.

Review labels

These labels allow us to quickly see what the review status of a PR is. These are set by the reviewer to show the review status.

Review: Feedback given

  • Pull request has got a review and needs some further work to be done before it can be merged. The PR has comments about the actions needed

Review: Ready to ship

  • Pull request is accepted for merging

@valscion valscion mentioned this pull request Feb 20, 2017
Closed
12 tasks
matthias-g
matthias-g approved these changes Feb 21, 2017
View reviewed changes
Copy link
Collaborator

@matthias-g matthias-g 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 to me. Thanks for checking for and adding the missing cases!

@valscion valscion merged commit b0b0101 into master Feb 21, 2017
@valscion valscion deleted the test-updates-for-authorize-relationships branch February 21, 2017 18:32
@valscion
Copy link
Member Author

valscion commented Feb 21, 2017

Thanks for the review :)

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

Reviewers

@matthias-g matthias-g matthias-g approved these changes

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@valscion @matthias-g