Skip to content

Conversation

@indocomsoft
Copy link
Member

Fixes #205

@coveralls
Copy link

coveralls commented Aug 13, 2018

Pull Request Test Coverage Report for Build 1487

  • 8 of 8 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.002%) to 99.723%

Totals Coverage Status
Change from base Build 1481: 0.002%
Covered Lines: 719
Relevant Lines: 721

💛 - Coveralls

Copy link
Member

@ning-y ning-y left a comment

Choose a reason for hiding this comment

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

As usual, specs looks good, but can't comment on the code.

Copy link
Contributor

@tuesmiddt tuesmiddt left a comment

Choose a reason for hiding this comment

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

Minor issue with the query.

Submission
|> where(status: ^:submitted)
|> join(:inner, [s], student in assoc(s, :student))
|> where([_, student], student.id == ^user_id)
Copy link
Contributor

Choose a reason for hiding this comment

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

No need for this. Submission has student_id field.

def user_max_grade(%User{id: user_id}) when is_ecto_id(user_id) do
max_grade =
Submission
|> where(status: ^:submitted)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is cool. Didn't know you could do that.

Copy link
Contributor

@tuesmiddt tuesmiddt left a comment

Choose a reason for hiding this comment

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

LGTM

@indocomsoft indocomsoft merged commit c6442ab into master Aug 13, 2018
@indocomsoft indocomsoft deleted the user-max-grade branch August 14, 2018 17:33
indocomsoft added a commit that referenced this pull request Aug 17, 2018
* Add maxGrade in UserInfo for submitted submissions

* Address code review
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.

5 participants