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

[SG-656] Update API endpoint to use RegisterResponseModel #2282

Merged
merged 1 commit into from
Sep 19, 2022

Conversation

addisonbeck
Copy link
Contributor

Type of change

- [x] Bug fix
- [ ] New feature development
- [ ] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [ ] Other

Objective

On #2278 I updated the Identity register endpoint to return a model, but I did not update the API register endpoint. This was because I was concerned they changes wouldn't be backwards compatible with old clients.

Turns out there is a bigger reason to update both register endpoints: those endpoints are still used by all self hosted servers. To keep self hosted registrations working I've updated the API register endpoint with the new changes. The changes are still backwards compatible with old clients because old clients will have marked the register endpoint as not having a response and will not try to parse one.

Code changes

  • Move the RegisterResponseModel classes to Core to be used by API and Identity
  • Copied the Identity register endpoint changes to the API register endpoint
  • Append the comments in this area to mention that self hosted installs still don't use the Identity endpoint
  • Add necessary dependencies to tests

Before you submit

  • Please check for formatting errors (dotnet format --verify-no-changes) (required)
  • If making database changes - make sure you also update Entity Framework queries and/or migrations
  • Please add unit tests where it makes sense to do so (encouraged but not required)
  • If this change requires a documentation update - notify the documentation team
  • If this change has particular deployment requirements - notify the DevOps team

@addisonbeck addisonbeck requested review from trmartin4 and a team September 16, 2022 20:23
}

#region DEPRECATED (Moved to Identity Service)

[Obsolete("2022-01-12 Moved to Identity, left for backwards compatability with older clients")]
// This method is still used by self hosted intalls
Copy link
Member

@differsthecat differsthecat Sep 16, 2022

Choose a reason for hiding this comment

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

TIL 😯

}

#region DEPRECATED (Moved to Identity Service)

[Obsolete("2022-01-12 Moved to Identity, left for backwards compatability with older clients")]
// This method is still used by self hosted intalls
Copy link
Member

Choose a reason for hiding this comment

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

It's actually used for all clients unless they run in dev since we needed to maintain backwards compatibility. I've created a PR to resolve this though since sufficient time has elapsed.

@addisonbeck addisonbeck merged commit d0c793c into master Sep 19, 2022
@addisonbeck addisonbeck deleted the fix/registration branch September 19, 2022 13:35
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.

None yet

3 participants