Skip to content

Add specs for ServiceProvider#live?#1161

Merged
monfresh merged 1 commit intomasterfrom
mb-service-provider-specs
Mar 2, 2017
Merged

Add specs for ServiceProvider#live?#1161
monfresh merged 1 commit intomasterfrom
mb-service-provider-specs

Conversation

@monfresh
Copy link
Copy Markdown
Contributor

@monfresh monfresh commented Mar 2, 2017

Why: To have complete code coverage.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

alpha order?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why do we have a live? method rather than just using active? ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I believe it's because we want to have the ability to approve SPs once we go live rather than automatically allow any SP that gets published on the dashboard. Currently, live? is not being used, but we want to be able to use it in the future. https://github.com/18F/identity-idp/blob/master/app/services/saml_request_validator.rb#L31

**Why**: To have complete code coverage.
@monfresh monfresh force-pushed the mb-service-provider-specs branch from 75b2afd to 4d61d72 Compare March 2, 2017 18:09
Copy link
Copy Markdown
Contributor

@jessieay jessieay left a comment

Choose a reason for hiding this comment

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

👍

@monfresh monfresh merged commit 983aac7 into master Mar 2, 2017
@monfresh monfresh deleted the mb-service-provider-specs branch March 2, 2017 19:14
class ServiceProviderUpdater
PROTECTED_ATTRIBUTES = [
:id,
:approved,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@monfresh what was the reason for making approved a protected attribute here? I think this may have resulted in a bug where we can't create new approved SPs from the dashboard app

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@zachmargolis is correct -- the approved flag should be a pass-through from the dashboard.

Copy link
Copy Markdown
Contributor Author

@monfresh monfresh Mar 6, 2017

Choose a reason for hiding this comment

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

Gotcha. I'm assuming the approved flag can only be set by admins on the dashboard, and that we prevent people from manually changing the form such that it submits the approved attribute?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

PR coming soon

amoose pushed a commit that referenced this pull request Mar 7, 2017
**Why**: To have complete code coverage.
amoose pushed a commit that referenced this pull request Mar 8, 2017
**Why**: To have complete code coverage.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants