Skip to content

Conversation

@ChristophWurst
Copy link
Member

This method has been on my list of code to refactor for a long time. As I now have to touch it for https://github.com/orgs/nextcloud/projects/29, I decided to break the code into smaller, maintainable and testable chunks. The code now follows a more strict single responsibility principle, while implementing the chain of responsibility pattern.

@ChristophWurst ChristophWurst added this to the Nextcloud 17 milestone May 3, 2019
@ChristophWurst ChristophWurst added 3. to review Waiting for reviews and removed 2. developing Work in progress labels May 6, 2019
@ChristophWurst ChristophWurst marked this pull request as ready for review May 6, 2019 11:53
@ChristophWurst ChristophWurst requested a review from rullzer May 6, 2019 11:55
@ChristophWurst ChristophWurst force-pushed the refactor/login-chain branch from f782905 to 17d6864 Compare May 6, 2019 11:55
@ChristophWurst ChristophWurst force-pushed the refactor/login-chain branch from 17d6864 to c868ce2 Compare May 6, 2019 13:20
@ChristophWurst ChristophWurst requested a review from MorrisJobke May 7, 2019 14:06
@ChristophWurst
Copy link
Member Author

From what I can tell CI failures do not look related. But tell me if I'm wrong.

@ChristophWurst
Copy link
Member Author

Redis::connect(): connect() failed: Connection refused

-> restarted

Copy link
Member

@rullzer rullzer left a comment

Choose a reason for hiding this comment

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

This is just great. So much cleaner. And tests 🚀

I didn't test in debt but most stuff didn't blow up with my smoke testing.

}

public function testNotTwoFactorAuthenticated() {
$this->fail('todo');
Copy link
Member Author

Choose a reason for hiding this comment

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

guess what … CI fails for a reason

Copy link
Member

Choose a reason for hiding this comment

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

NO WAY!

@ChristophWurst ChristophWurst force-pushed the refactor/login-chain branch from c868ce2 to 170582d Compare May 7, 2019 16:05
Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

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

Tested and works 👍

@MorrisJobke MorrisJobke added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels May 7, 2019
@MorrisJobke MorrisJobke merged commit a6a8f85 into master May 7, 2019
@MorrisJobke MorrisJobke deleted the refactor/login-chain branch May 7, 2019 20:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

4. to release Ready to be released and/or waiting for tests to finish technical debt

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants