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

Authentication controller v2 migration #10950

Merged
merged 22 commits into from
Aug 1, 2019

Conversation

naz
Copy link
Contributor

@naz naz commented Jul 25, 2019

refs #10060

Work in progress. But all methods are moved already.

One change think is worth doing while at this stage is splitting this authentication controller into more natural pieces - setup, invitations (we already have invites controller) and passwordreset. Although they partially fit under the same 'authentication' umbrella, think it's worth to break the authentication controller further for couple reasons:

  1. It is touching different areas
  2. Having to use custom docName: 'authentication' under most endpoints
  3. Having too many methods in a single controller

The other bit is the naming of endpoints. Currently, these are:

router.post('/authentication/passwordreset', ..., api.http(apiv2.authentication.generateResetToken));
router.put('/authentication/passwordreset', ..., api.http(apiv2.authentication.resetPassword));
router.post('/authentication/invitation', api.http(apiv2.authentication.acceptInvitation));
router.get('/authentication/invitation', api.http(apiv2.authentication.isInvitation));
router.post('/authentication/setup', api.http(apiv2.authentication.setup));
router.put('/authentication/setup', ..., api.http(apiv2.authentication.updateSetup));
router.get('/authentication/setup', api.http(apiv2.authentication.isSetup));

To me it feels like simplifying it down to plain names like /passwordreset/ without /authentication/ prefix would make sense?

@kevinansfield @allouis would love to hear your thoughts on the above 💃

Todo:

  • manual testing
  • probable controller breakdown to multiple pieces

@kirrg001 kirrg001 mentioned this pull request Jul 25, 2019
12 tasks
naz added 9 commits July 30, 2019 15:42
- Without the event it's possible to simplify sendNotification method to just use email address of the user
- The only thing the method does now is sending welcome mail, so new naming seems natural :)
- This is the code corresponding to processArgs function in v1 authentication.updateSetup method
- Added missing endpoint coverage
- Minor fixes with formatting and validations uncovered by the test
- Added same test to v0.1 coverage
@naz naz marked this pull request as ready for review July 30, 2019 21:18
@naz
Copy link
Contributor Author

naz commented Jul 30, 2019

@allouis @kevinansfield ping 😉

@allouis
Copy link
Contributor

allouis commented Jul 31, 2019

This all looks good to me!

  1. Naming - I don't have any opinion on this either way tbh 🤷‍♂️

  2. Splitting the controller out - I personally think we don't need to - it's not crazy big yet.

IMO this is good to merge as is 👍

@kevinansfield
Copy link
Contributor

LGTM 👍

To me it feels like simplifying it down to plain names like /passwordreset/ without /authentication/ prefix would make sense?

I don't think we should change anything at the moment especially as it would be a breaking change for v2. It's worth thinking about a little for v3 but I don't think it's a pressing need, namespacing under /authentication/ helps keep the top-level namespace clearer 🤔

One change think is worth doing while at this stage is splitting this authentication controller into more natural pieces

It's fine as it is I think, these are (and should be) rarely changing bits of code and it's useful to see all endpoints for the setup/invite/signup flow together. I do get your point though, we can look at refactoring+splitting if it starts to cause pain.

- Small adjustments in controller that came along with the switch
@naz naz merged commit 27bf453 into TryGhost:master Aug 1, 2019
@naz naz deleted the authentication-controller-v2-migration branch August 1, 2019 11:18
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