Skip to content

Conversation

@geshuming
Copy link
Contributor

No description provided.

@coveralls
Copy link

coveralls commented May 30, 2019

Pull Request Test Coverage Report for Build 2458

  • 37 of 41 (90.24%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.009%) to 97.488%

Changes Missing Coverage Covered Lines Changed/Added Lines %
lib/cadet_web/views/assessments_helpers.ex 31 35 88.57%
Totals Coverage Status
Change from base Build 2456: 0.009%
Covered Lines: 815
Relevant Lines: 836

💛 - Coveralls

@geshuming geshuming requested a review from jiachen247 June 2, 2019 12:15
Copy link
Contributor

@jiachen247 jiachen247 left a comment

Choose a reason for hiding this comment

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

Heyy shuming!! PR looks good!! love how small it is :-)

  1. Everytime we change the view layer we have to update our swagger documentation they can be found in the respective controllers and accessed over /swagger on the backend.

  2. Would be nice to have a little writeup in the PR to know what the PR is intended for!! so we can look back and make sense of it and so the reviewer sort of knows what to look out for!!

But other than that good job!! will take a look at the frontend next haha cheers :-)

end

defp build_result(result) do
def build_result(result) do
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this belongs in view_helpers now since its being shared?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should still be fine leaving it under assessments_view? At most only grading_view and assessments_view require build_result so I don't think it makes sense to isolate the function.

Copy link
Contributor

Choose a reason for hiding this comment

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

the pattern we have been adopting is all shared methods go into the view helpers so our views dont couple or depend on other views!! lets try to be consistent with the codebase okay!!

@jiachen247 jiachen247 changed the title Added autogradingResults to grading_view Add autograding results to gradingview Jun 5, 2019
Copy link
Contributor

@jiachen247 jiachen247 left a comment

Choose a reason for hiding this comment

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

LGTM

@geshuming geshuming merged commit 1c9d806 into source-academy:master Jun 14, 2019
@geshuming geshuming deleted the add-autograding-results branch June 15, 2019 15:09
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.

3 participants