Skip to content

Refactor User#update_last_seen! and User#update_tracked_topics#1571

Merged
eviltrout merged 1 commit intodiscourse:masterfrom
novemberkilo:master
Oct 29, 2013
Merged

Refactor User#update_last_seen! and User#update_tracked_topics#1571
eviltrout merged 1 commit intodiscourse:masterfrom
novemberkilo:master

Conversation

@novemberkilo
Copy link
Copy Markdown
Contributor

Improves the score of the User class by refactoring two of its most complex methods (per code climate).

Refactors User#update_last_seen! and User#update_tracked_topics

(This PR was written while wearing a Discourse t-shirt - thanks @SamSaffron 😄)

@discoursebot
Copy link
Copy Markdown

You've signed the CLA, novemberkilo. Thank you! This pull request is ready for review.

@eviltrout
Copy link
Copy Markdown
Contributor

I've reviewed this PR and it looks good but one thing that scares me is the test coverage for update_last_seen! seems to be poor. How did you test that it works? In browser, looking at the database?

@novemberkilo
Copy link
Copy Markdown
Contributor Author

I relied on not breaking any of the current test suite.

@SamSaffron
Copy link
Copy Markdown
Member

thanks @novemberkilo

I agree with @eviltrout here, any way we can increase / improve test coverage here?

@novemberkilo
Copy link
Copy Markdown
Contributor Author

@eviltrout @SamSaffron I've generally attempted 'code cleanup' PRs as refactoring exercises that use the current test spec as a marker for whether the refactored code is at least as valid as the code it is replacing. I'm afraid I don't know the business rules for User#update_last_seen! so am not in a position to add test coverage. I know it it tested indirectly through integration tests (because I broke some of them while refactoring, and used them as a way to fixing my code). I'm sorry but I feel there isn't much more I can do here unless I'm given some more direction.

@eviltrout
Copy link
Copy Markdown
Contributor

Actually I looked again and the coverage is not that bad. Our autotester had a bug that didn't refresh the class properly and I thought it was much worse. I'm happy to accept this, thanks!

eviltrout added a commit that referenced this pull request Oct 29, 2013
Refactor User#update_last_seen! and User#update_tracked_topics
@eviltrout eviltrout merged commit f7d6ab5 into discourse:master Oct 29, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants