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

OpenID Recipes: use step model instead of view model, support update #10136

Merged
merged 8 commits into from
Oct 3, 2021
Merged

OpenID Recipes: use step model instead of view model, support update #10136

merged 8 commits into from
Oct 3, 2021

Conversation

kaipm
Copy link
Contributor

@kaipm kaipm commented Aug 17, 2021

Changes in the pull request

  • Add unit tests for OpenID scopes
  • Add unit tests for OpenID apps
  • Replace OpenIdScopeStepViewModel with OpenIdScopeStepModel in recipe
  • Replace CreateOpenIdApplicationViewModel with OpenIdApplicationStepModel in recipe
  • Adjust OpenIdScopeStep to support update
  • Adjust OpenIdApplicationStep to support update
  • Adjust OpenIdApplicationStep to include importing scopes, which were not imported before

This is another part of #6364

@kaipm kaipm requested a review from kevinchalet as a code owner August 17, 2021 18:33
@kaipm kaipm closed this Aug 21, 2021
@kaipm kaipm reopened this Aug 22, 2021
@kaipm kaipm changed the title OpenID Scope Recipe: use step model instead of view model, support update OpenID Recipes: use step model instead of view model, support update Aug 22, 2021
Copy link
Member

@deanmarcussen deanmarcussen left a comment

Choose a reason for hiding this comment

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

Are the new step models the same as the old viewmodels that were used?

i.e. is it non breaking on existing recipes, or?

But awesome looks really good thanks

@sebastienros
Copy link
Member

@kaipm we just need to understand if it is breaking anything so we can document the upgrade steps accordingly.

@kaipm
Copy link
Contributor Author

kaipm commented Sep 23, 2021

Yes, they are. I did unit tests before making any changes, to make sure I match the old structure.

However, there is a small difference in the behaviour:

The following example was necessary to add role1 before the PR, it still works after the PR:

"roleEntries": [
  {
    "name": "role1",
    "selected": true
  }
]

The following two examples did not add role1 before the PR. But after the PR they will add role1 because selected is ignored completely:

"roleEntries": [
  {
    "name": "role1",
    "selected": false
  }
]
"roleEntries": [
  {
    "name": "role1"
  }
]

Edit: I forgot, OpenID Scopes were not imported at all, so those were newly added. I made them similar to the role entries, to be a little consistent:

"scopeEntries": [
  {
    "name": "scope1"
  }
]

Copy link
Member

@hishamco hishamco left a comment

Choose a reason for hiding this comment

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

LGTM, after removing the extra whitespaces

@kaipm
Copy link
Contributor Author

kaipm commented Oct 2, 2021

@hishamco, I did all the proposed changes.

I also removed some line breaks between IF-statements too, because most were without already. I hope that's fine!

@hishamco
Copy link
Member

hishamco commented Oct 2, 2021

Thanks @kaipm for the PR, we will merge this ASAP

1 similar comment
@hishamco

This comment has been minimized.

@deanmarcussen deanmarcussen merged commit a1274bb into OrchardCMS:main Oct 3, 2021
@kaipm kaipm deleted the refactor-openid-recipes branch October 4, 2021 16:26
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.

4 participants