Implement Updater as a GenServer #74
Conversation
Pull Request Test Coverage Report for Build 640
💛 - Coveralls |
|
I think you should rebase this on |
|
@indocomsoft Rebased, but you'd probably want to look at #76 first, since this PR uses one commit from that branch (90497ee). |
indocomsoft
left a comment
indocomsoft
left a comment
There was a problem hiding this comment.
Good job on using GenServer! Just some refactors here and there
lib/cadet/accounts/ivle.ex
Outdated
| url = | ||
| @api_url | ||
| |> URI.merge(method) | ||
| |> (&"#{&1}?#{URI.encode_query(queries)}").() |
| config :cadet, ecto_repos: [Cadet.Repo] | ||
| config :cadet, | ||
| ecto_repos: [Cadet.Repo], | ||
| updater: [interval: 5 * 60 * 1000] |
There was a problem hiding this comment.
This might be better:
alias Timex.Duration
5 |> Duration.from_minutes() |> Duration.to_milliseconds()There was a problem hiding this comment.
Can't easily use Timex in the config, since config is evaluated before dependencies are compiled. I don't think it's worth the trouble to implement a work around just to be able to use this pipe. Added a comment indicating that the value for interval is in milliseconds in fd9de8e.
lib/cadet/public/updater.ex
Outdated
| WARNING: The GenServer crashes if the API key is invalid, or not provided. | ||
| """ | ||
| def start_link() do | ||
| GenServer.start_link(__MODULE__, %{}) |
There was a problem hiding this comment.
If init/1 ignores its argument, then maybe change %{} to nil?
lib/cadet/public/updater.ex
Outdated
| `schedule_work/0` -> `handle_info/2` -> ... | ||
| """ | ||
| def init(_) do | ||
| token = get_token() |
There was a problem hiding this comment.
I think you can refactor out get_api_params/0
lib/cadet/public/updater.ex
Outdated
| {:error, :bad_request} -> | ||
| # the token has probably expired---get a new one | ||
| Logger.info("Updater failed fetching announcements. Refreshing token...") | ||
| token = get_token() |
There was a problem hiding this comment.
Refactor to get_api_params/0
test/cadet/public/updater_test.exs
Outdated
| assert {:ok, pid} = Updater.start_link() | ||
| assert Process.alive?(pid) == true | ||
| assert GenServer.stop(pid) == :ok | ||
| assert Process.alive?(pid) == false |
There was a problem hiding this comment.
Instead do refute Process.alive?(pid)
test/cadet/public/updater_test.exs
Outdated
|
|
||
| test "GenServer init/1 callback" do | ||
| use_cassette "updater/init#1", custom: true do | ||
| assert {:ok, %{token: token, course_id: course_id}} = Updater.init(%{}) |
There was a problem hiding this comment.
Change %{} to nil if it is ignored
test/cadet/public/updater_test.exs
Outdated
| assert {:ok, %{token: token, course_id: course_id}} = Updater.init(%{}) | ||
| assert String.length(token) == 480 | ||
| assert String.length(course_id) == 36 | ||
| assert course_id != "00000000-0000-0000-0000-000000000000" |
test/cadet/public/updater_test.exs
Outdated
| use_cassette "updater/handle_info#1", custom: true do | ||
| token = Updater.get_token() | ||
| course_id = Updater.get_course_id(token) | ||
| api_params = %{token: token, course_id: course_id} |
There was a problem hiding this comment.
Once get_api_params/0 is refactored, use that instead here.
test/cadet/public/updater_test.exs
Outdated
| use_cassette "updater/handle_info#2", custom: true do | ||
| token = Updater.get_token() | ||
| course_id = Updater.get_course_id(token) | ||
| api_params = %{token: "bad_token", course_id: course_id} |
ning-y
force-pushed
the
updater-state
branch
2 times, most recently
from
a2ae3d9 to
5147d95
Compare
|
Adding tests for new e: Done with 8a56a35. |
|
Test coverage fell, seems e: Fixed with 6196dc9. |
ning-y
force-pushed
the
updater-state
branch
2 times, most recently
from
de4b70d to
3f878c2
Compare
|
@indocomsoft We need to hold off on this merge until I implement some sort of flag that decides if the GenServer should be run. The issue is there are at least two ec2 instances running this application simultaneously because of the AWS auto-scaling group. As a result, there would be at least twice the expected number of calls to IVLE due to this GenServer. This is also a problem when we have to implement automated grading later on. Only one 'main' instance of this application should be the one checking the deadline and sending out requests to the AWS lambda auto grader. Otherwise, we end up duplication calls. e: Done with 9c3a157. |
|
@indocomsoft Rebased and adding command line options to start the updater. |
|
Might wanna add notice on README to warn people that changing |
| """ | ||
| def api_call(method, queries) do | ||
| case HTTPoison.get(api_url(method, queries)) do | ||
| {:ok, %{body: body, status_code: 200}} when body != ~s("") -> |
There was a problem hiding this comment.
Instead of doing
{:ok, %{body: body, status_code: 200}} when body != ~s("") ->
{:ok, %{body: ~s(""), status_code: 200}} ->Maybe you could consider doing
{:ok, %{body: ~s(""), status_code: 200}} ->
{:ok, %{body: body, status_code: 200}} ->Since case pattern matching is evaluated sequentially
There was a problem hiding this comment.
I prefer the current match order, since it follows an 'if ok, else error' structure, rather than an 'if error, elif ok, else error'.
|
|
||
| # To supply command line args to phx.server, you must use the elixir/iex bin | ||
| # $ elixir --erl "--updater" -S mix phx.server | ||
| # $ iex --erl "--updater" -S mix phx.server |
There was a problem hiding this comment.
Should be cadet.server me thinks
| {:ok, modules} = api_fetch("Modules", AuthToken: token, CourseID: "CS1101S") | ||
| {:ok, modules} = api_call("Modules", AuthToken: token, CourseID: "CS1101S") | ||
|
|
||
| cs1101s = |
There was a problem hiding this comment.
Not sure if the
cs1101s = modules["Results"] |> Enum.find part should be refactored to a separate method. Currently there are only 2 occurrences of them, so let's keep them separate for now following the rule of 3
|
@indocomsoft There's already instructions about recompilation in the current README.
|
* Add Updater.get_course_id/1 * Add test for Updater.get_course_id/1 * Add tests for IVLE.api_fetch/2 * Make Updater implement GenServer * Fix invalid token failing silently (announcements) * Add Updater to the supervisor tree * Rename IVLE's api_fetch/2 -> api_call/2 * Update documentation * Add test for new guarded IVLE.api_call/2 * Add test for Updater.get_announcements#1 * Add tests for Updater.init/1 * Add tests for Updater.handle_info/2 * Fix missing cassette in updater_test * Refactor a line with pipe operators * Improve test coverage for Updater * Fix private function being public * Bump test coverage for Updater to 100% * Document interval being milliseconds with comments * Refactor updater * Remove redundant case block * Replace Enum.filter with Enum.find * Refactor tests * Add test for get_api_params/0 * Fix reduced test coverage * Add command line flag to toggle updater
Features
--updaterelixir --erl "--updater" -S mix phx.server. (oriex)bin/cadet start --updaterhandle_info/2(:work, ...).schedule_work/0, which callshandle_info/2(:work, ...), which callsschedule_work/0, and so on