From 14edd31b9d7f2afde20e10eb8d72f1fd0bd1cd9a Mon Sep 17 00:00:00 2001 From: Stamates Date: Thu, 14 Jun 2018 14:38:12 -0400 Subject: [PATCH] Change to last seen paging and hide paging button on last page for recent_pending_transactions --- apps/explorer/lib/explorer/chain.ex | 148 ++++++------------ apps/explorer/test/explorer/chain_test.exs | 96 ++++++------ .../pending_transaction_controller.ex | 51 ++++-- .../pending_transaction/index.html.eex | 23 ++- .../pending_transaction_controller_test.exs | 53 +++++-- 5 files changed, 178 insertions(+), 193 deletions(-) diff --git a/apps/explorer/lib/explorer/chain.ex b/apps/explorer/lib/explorer/chain.ex index 22136743bc..cd3a897d6c 100644 --- a/apps/explorer/lib/explorer/chain.ex +++ b/apps/explorer/lib/explorer/chain.ex @@ -55,15 +55,8 @@ defmodule Explorer.Chain do """ @type necessity_by_association :: %{association => necessity} - @typedoc """ - Pagination params used by `scrivener` - """ - @type pagination :: map() - - @typep inserted_after_option :: {:inserted_after, DateTime.t()} @typep necessity_by_association_option :: {:necessity_by_association, necessity_by_association} @typep on_conflict_option :: {:on_conflict, :nothing | :replace_all} - @typep pagination_option :: {:pagination, pagination} @typep paging_options :: {:paging_options, PagingOptions.t()} @typep params_option :: {:params, [map()]} @typep timeout_option :: {:timeout, timeout} @@ -1947,12 +1940,12 @@ defmodule Explorer.Chain do end @doc """ - Return the list of pending transactions that occurred recently (10). + Return the list of pending transactions that occurred recently (8). iex> 2 |> insert_list(:transaction) iex> :transaction |> insert() |> with_block() iex> 8 |> insert_list(:transaction) - iex> %Scrivener.Page{entries: recent_pending_transactions} = Explorer.Chain.recent_pending_transactions() + iex> recent_pending_transactions = Explorer.Chain.recent_pending_transactions() iex> length(recent_pending_transactions) 10 iex> Enum.all?(recent_pending_transactions, fn %Explorer.Chain.Transaction{block_hash: block_hash} -> @@ -1960,69 +1953,28 @@ defmodule Explorer.Chain do ...> end) true - A `t:Explorer.Chain.Transaction.t/0` `inserted_at` can be supplied to the `:inserted_after` option, then only pending - transactions inserted after that transaction will be returned. This can be used to generate paging for pending - transactions. - - iex> {:ok, first_inserted_at, 0} = DateTime.from_iso8601("2015-01-23T23:50:07Z") - iex> insert(:transaction, inserted_at: first_inserted_at) - iex> {:ok, second_inserted_at, 0} = DateTime.from_iso8601("2016-01-23T23:50:07Z") - iex> insert(:transaction, inserted_at: second_inserted_at) - iex> %Scrivener.Page{entries: after_first_transaction} = Explorer.Chain.recent_pending_transactions( - ...> inserted_after: first_inserted_at - ...> ) - iex> length(after_first_transaction) - 1 - iex> %Scrivener.Page{entries: after_second_transaction} = Explorer.Chain.recent_pending_transactions( - ...> inserted_after: second_inserted_at - ...> ) - iex> length(after_second_transaction) - 0 - - When there are no pending transaction and a collated transaction's inserted_at is used, an empty list is returned - - iex> {:ok, first_inserted_at, 0} = DateTime.from_iso8601("2015-01-23T23:50:07Z") - iex> :transaction |> insert(inserted_at: first_inserted_at) |> with_block() - iex> {:ok, second_inserted_at, 0} = DateTime.from_iso8601("2016-01-23T23:50:07Z") - iex> :transaction |> insert(inserted_at: second_inserted_at) |> with_block() - iex> %Scrivener.Page{entries: entries} = Explorer.Chain.recent_pending_transactions( - ...> after_inserted_at: first_inserted_at - ...> ) - iex> entries - [] - ## Options * `:necessity_by_association` - use to load `t:association/0` as `:required` or `:optional`. If an association is `:required`, and the `t:Explorer.Chain.InternalTransaction.t/0` has no associated record for that association, then the `t:Explorer.Chain.InternalTransaction.t/0` will not be included in the list. - * `:pagination` - pagination params to pass to scrivener. + * `:paging_options` - a `t:Explorer.PagingOptions.t/0` used to specify the `:page_size` and + `:key` (a tuple of the lowest/oldest {inserted_at, hash}) and. Results will be the transactions older than + the inserted_at and hash that are passed. """ - @spec recent_pending_transactions([inserted_after_option | necessity_by_association_option]) :: %Scrivener.Page{ - entries: [Transaction.t()] - } + @spec recent_pending_transactions([paging_options | necessity_by_association_option]) :: [Transaction.t()] def recent_pending_transactions(options \\ []) when is_list(options) do necessity_by_association = Keyword.get(options, :necessity_by_association, %{}) - pagination = Keyword.get(options, :pagination, %{}) - - query = - from( - transaction in Transaction, - where: is_nil(transaction.block_hash), - order_by: [ - desc: transaction.inserted_at, - # arbitary tie-breaker when inserted at is the same. hash is random distribution, but using it keeps order - # consistent at least - desc: transaction.hash - ], - limit: 10 - ) + paging_options = Keyword.get(options, :paging_options, %PagingOptions{page_size: 50}) - query - |> inserted_after(options) + Transaction + |> where([transaction], is_nil(transaction.block_hash)) + |> page_pending_transaction(paging_options) + |> limit(^paging_options.page_size) + |> order_by([transaction], desc: transaction.inserted_at, desc: transaction.hash) |> join_associations(necessity_by_association) - |> Repo.paginate(pagination) + |> Repo.all() end @doc """ @@ -2154,14 +2106,28 @@ defmodule Explorer.Chain do * `:necessity_by_association` - use to load `t:association/0` as `:required` or `:optional`. If an association is `:required`, and the `t:Explorer.Chain.Log.t/0` has no associated record for that association, then the `t:Explorer.Chain.Log.t/0` will not be included in the page `entries`. - * `:pagination` - pagination params to pass to scrivener. + * `:paging_options` - a `t:Explorer.PagingOptions.t/0` used to specify the `:page_size` and + `:key` (a tuple of the lowest/oldest {index}) and. Results will be the transactions older than + the index that are passed. """ - @spec transaction_to_logs(Transaction.t(), [ - necessity_by_association_option | pagination_option - ]) :: %Scrivener.Page{entries: [Log.t()]} - def transaction_to_logs(%Transaction{hash: hash}, options \\ []) when is_list(options) do - transaction_hash_to_logs(hash, options) + @spec transaction_to_logs(Transaction.t(), [paging_options | necessity_by_association_option]) :: [Log.t()] + def transaction_to_logs( + %Transaction{hash: %Hash{byte_count: unquote(Hash.Full.byte_count())} = transaction_hash}, + options \\ [] + ) + when is_list(options) do + necessity_by_association = Keyword.get(options, :necessity_by_association, %{}) + paging_options = Keyword.get(options, :paging_options, %PagingOptions{page_size: 50}) + + Log + |> join(:inner, [log], transaction in assoc(log, :transaction)) + |> where([_, transaction], transaction.hash == ^transaction_hash) + |> page_logs(paging_options) + |> limit(^paging_options.page_size) + |> order_by([log], asc: log.index) + |> join_associations(necessity_by_association) + |> Repo.all() end @doc """ @@ -2495,16 +2461,6 @@ defmodule Explorer.Chain do {:ok, for(transaction <- transactions, do: transaction.hash)} end - defp inserted_after(query, options) do - case Keyword.fetch(options, :inserted_after) do - {:ok, inserted_after} -> - from(transaction in query, where: ^inserted_after < transaction.inserted_at) - - :error -> - query - end - end - defp join_association(query, association, necessity) when is_atom(association) do case necessity do :optional -> @@ -2556,6 +2512,23 @@ defmodule Explorer.Chain do |> where([internal_transaction], internal_transaction.index < ^index) end + defp page_logs(query, %PagingOptions{key: nil}), do: query + + defp page_logs(query, %PagingOptions{key: {index}}) do + query + |> where([log], log.index < ^index) + end + + defp page_pending_transaction(query, %PagingOptions{key: nil}), do: query + + defp page_pending_transaction(query, %PagingOptions{key: {inserted_at, hash}}) do + query + |> where( + [transaction], + transaction.inserted_at < ^inserted_at or (transaction.inserted_at == ^inserted_at and transaction.hash < ^hash) + ) + end + defp page_transaction(query, %PagingOptions{key: nil}), do: query defp page_transaction(query, %PagingOptions{key: {block_number, index}}) do @@ -2685,27 +2658,6 @@ defmodule Explorer.Chain do %{inserted_at: now, updated_at: now} end - defp transaction_hash_to_logs( - %Hash{byte_count: unquote(Hash.Full.byte_count())} = transaction_hash, - options - ) - when is_list(options) do - necessity_by_association = Keyword.get(options, :necessity_by_association, %{}) - pagination = Keyword.get(options, :pagination, %{}) - - query = - from( - log in Log, - join: transaction in assoc(log, :transaction), - where: transaction.hash == ^transaction_hash, - order_by: [asc: :index] - ) - - query - |> join_associations(necessity_by_association) - |> Repo.paginate(pagination) - end - defp where_address_fields_match(query, address_hash, direction \\ nil) do address_fields = case direction do diff --git a/apps/explorer/test/explorer/chain_test.exs b/apps/explorer/test/explorer/chain_test.exs index d380494446..fa4f6678c4 100644 --- a/apps/explorer/test/explorer/chain_test.exs +++ b/apps/explorer/test/explorer/chain_test.exs @@ -524,6 +524,33 @@ defmodule Explorer.ChainTest do end end + describe "pending_transactions/0" do + test "without transactions" do + assert [] = Chain.recent_pending_transactions() + end + + test "with transactions" do + %Transaction{hash: hash} = insert(:transaction) + + assert [%Transaction{hash: ^hash}] = Chain.recent_pending_transactions() + end + + test "with transactions can be paginated" do + second_page_hashes = + 50 + |> insert_list(:transaction) + |> Enum.map(& &1.hash) + + %Transaction{inserted_at: inserted_at, hash: hash} = insert(:transaction) + + assert second_page_hashes == + [paging_options: %PagingOptions{key: {inserted_at, hash}, page_size: 50}] + |> Chain.recent_pending_transactions() + |> Enum.map(& &1.hash) + |> Enum.reverse() + end + end + describe "transaction_to_internal_transactions/1" do test "with transaction without internal transactions" do transaction = insert(:transaction) @@ -623,12 +650,7 @@ defmodule Explorer.ChainTest do test "without logs" do transaction = insert(:transaction) - assert %Scrivener.Page{ - entries: [], - page_number: 1, - total_entries: 0, - total_pages: 1 - } = Chain.transaction_to_logs(transaction) + assert [] = Chain.transaction_to_logs(transaction) end test "with logs" do @@ -639,12 +661,7 @@ defmodule Explorer.ChainTest do %Log{id: id} = insert(:log, transaction: transaction) - assert %Scrivener.Page{ - entries: [%Log{id: ^id}], - page_number: 1, - total_entries: 1, - total_pages: 1 - } = Chain.transaction_to_logs(transaction) + assert [%Log{id: ^id}] = Chain.transaction_to_logs(transaction) end test "with logs can be paginated" do @@ -653,25 +670,17 @@ defmodule Explorer.ChainTest do |> insert() |> with_block() - logs = Enum.map(0..1, &insert(:log, index: &1, transaction: transaction)) + second_page_indexes = + 1..50 + |> Enum.map(fn index -> insert(:log, transaction_hash: transaction.hash, index: index) end) + |> Enum.map(& &1.index) - [%Log{id: first_log_id}, %Log{id: second_log_id}] = logs + log = insert(:log, transaction_hash: transaction.hash, index: 51) - assert %Scrivener.Page{ - entries: [%Log{id: ^first_log_id}], - page_number: 1, - page_size: 1, - total_entries: 2, - total_pages: 2 - } = Chain.transaction_to_logs(transaction, pagination: %{page_size: 1}) - - assert %Scrivener.Page{ - entries: [%Log{id: ^second_log_id}], - page_number: 2, - page_size: 1, - total_entries: 2, - total_pages: 2 - } = Chain.transaction_to_logs(transaction, pagination: %{page: 2, page_size: 1}) + assert second_page_indexes == + transaction + |> Chain.transaction_to_logs(paging_options: %PagingOptions{key: {log.index}, page_size: 50}) + |> Enum.map(& &1.index) end test "with logs necessity_by_association loads associations" do @@ -682,17 +691,7 @@ defmodule Explorer.ChainTest do insert(:log, transaction: transaction) - assert %Scrivener.Page{ - entries: [ - %Log{ - address: %Address{}, - transaction: %Transaction{} - } - ], - page_number: 1, - total_entries: 1, - total_pages: 1 - } = + assert [%Log{address: %Address{}, transaction: %Transaction{}}] = Chain.transaction_to_logs( transaction, necessity_by_association: %{ @@ -701,17 +700,12 @@ defmodule Explorer.ChainTest do } ) - assert %Scrivener.Page{ - entries: [ - %Log{ - address: %Ecto.Association.NotLoaded{}, - transaction: %Ecto.Association.NotLoaded{} - } - ], - page_number: 1, - total_entries: 1, - total_pages: 1 - } = Chain.transaction_to_logs(transaction) + assert [ + %Log{ + address: %Ecto.Association.NotLoaded{}, + transaction: %Ecto.Association.NotLoaded{} + } + ] = Chain.transaction_to_logs(transaction) end end diff --git a/apps/explorer_web/lib/explorer_web/controllers/pending_transaction_controller.ex b/apps/explorer_web/lib/explorer_web/controllers/pending_transaction_controller.ex index 19fc582cb6..90bb3711f4 100644 --- a/apps/explorer_web/lib/explorer_web/controllers/pending_transaction_controller.ex +++ b/apps/explorer_web/lib/explorer_web/controllers/pending_transaction_controller.ex @@ -1,35 +1,54 @@ defmodule ExplorerWeb.PendingTransactionController do use ExplorerWeb, :controller - alias Explorer.Chain + alias Explorer.{Chain, PagingOptions} + # alias Explorer.Chain.Hash + + @page_size 50 + @default_paging_options %PagingOptions{page_size: @page_size + 1} def index(conn, params) do - with %{"last_seen_pending_inserted_at" => last_seen_pending_inserted_at_string} <- params, - {:ok, last_seen_pending_inserted_at} = Timex.parse(last_seen_pending_inserted_at_string, "{ISO:Extended:Z}") do - do_index(conn, inserted_after: last_seen_pending_inserted_at) - else - _ -> do_index(conn) - end - end + full_options = + Keyword.merge( + [ + necessity_by_association: %{ + from_address: :optional, + to_address: :optional + } + ], + paging_options(params) + ) + + transactions_plus_one = Chain.recent_pending_transactions(full_options) + + {transactions, next_page} = Enum.split(transactions_plus_one, @page_size) - defp do_index(conn, options \\ []) when is_list(options) do - full_options = Keyword.merge([necessity_by_association: %{from_address: :optional, to_address: :optional}], options) - transactions = Chain.recent_pending_transactions(full_options) - last_seen_pending_inserted_at = last_seen_pending_inserted_at(transactions.entries) pending_transaction_count = Chain.pending_transaction_count() render( conn, "index.html", - last_seen_pending_inserted_at: last_seen_pending_inserted_at, + next_page_params: next_page_params(next_page, transactions), pending_transaction_count: pending_transaction_count, transactions: transactions ) end - defp last_seen_pending_inserted_at([]), do: nil + defp next_page_params([], _transactions), do: nil + + defp next_page_params(_, transactions) do + last = List.last(transactions) + %{inserted_at: DateTime.to_iso8601(last.inserted_at), hash: last.hash} + end - defp last_seen_pending_inserted_at(transactions) do - List.last(transactions).inserted_at + defp paging_options(params) do + with %{"inserted_at" => inserted_at_string, "hash" => hash_string} <- params, + {:ok, inserted_at, _} <- DateTime.from_iso8601(inserted_at_string), + {:ok, hash} <- Chain.string_to_transaction_hash(hash_string) do + [paging_options: %{@default_paging_options | key: {inserted_at, hash}}] + else + _ -> + [paging_options: @default_paging_options] + end end end diff --git a/apps/explorer_web/lib/explorer_web/templates/pending_transaction/index.html.eex b/apps/explorer_web/lib/explorer_web/templates/pending_transaction/index.html.eex index 9e0d610ef9..4e3a36fb2d 100644 --- a/apps/explorer_web/lib/explorer_web/templates/pending_transaction/index.html.eex +++ b/apps/explorer_web/lib/explorer_web/templates/pending_transaction/index.html.eex @@ -64,19 +64,16 @@ - <%= if @last_seen_pending_inserted_at do %> + <%= if @next_page_params do %> <%= link( - gettext("Next Page"), - class: "button button--secondary button--sm u-float-right mt-3", - to: pending_transaction_path( - @conn, - :index, - @conn.assigns.locale, - %{ - "last_seen_inserted_at" => - Timex.format!(@last_seen_pending_inserted_at, "{ISO:Extended:Z}") - } - ) - ) %> + gettext("Older"), + class: "button button--secondary button--sm u-float-right mt-3", + to: pending_transaction_path( + @conn, + :index, + @conn.assigns.locale, + @next_page_params + ) + ) %> <% end %> diff --git a/apps/explorer_web/test/explorer_web/controllers/pending_transaction_controller_test.exs b/apps/explorer_web/test/explorer_web/controllers/pending_transaction_controller_test.exs index 0fdbe4edaa..d5bc7bd503 100644 --- a/apps/explorer_web/test/explorer_web/controllers/pending_transaction_controller_test.exs +++ b/apps/explorer_web/test/explorer_web/controllers/pending_transaction_controller_test.exs @@ -1,5 +1,6 @@ defmodule ExplorerWeb.PendingTransactionControllerTest do use ExplorerWeb.ConnCase + alias Explorer.Chain.{Hash, Transaction} import ExplorerWeb.Router.Helpers, only: [pending_transaction_path: 3] @@ -48,27 +49,49 @@ defmodule ExplorerWeb.PendingTransactionControllerTest do assert 1 == conn.assigns.pending_transaction_count end - test "paginates transactions using the last seen transaction", %{conn: conn} do - {:ok, first_inserted_at, 0} = DateTime.from_iso8601("2015-01-23T23:50:07Z") - insert(:transaction, inserted_at: first_inserted_at) - {:ok, second_inserted_at, 0} = DateTime.from_iso8601("2016-01-23T23:50:07Z") - insert(:transaction, inserted_at: second_inserted_at) + test "works when there are no transactions", %{conn: conn} do + conn = get(conn, pending_transaction_path(conn, :index, :en)) + + assert html_response(conn, 200) + end + + test "returns next page of results based on last seen pending transaction", %{conn: conn} do + second_page_hashes = + 50 + |> insert_list(:transaction) + |> Enum.map(& &1.hash) + + %Transaction{inserted_at: inserted_at, hash: hash} = insert(:transaction) conn = - get( - conn, - pending_transaction_path(ExplorerWeb.Endpoint, :index, :en), - last_seen_pending_inserted_at: Timex.format!(first_inserted_at, "{ISO:Extended:Z}") - ) + get(conn, pending_transaction_path(ExplorerWeb.Endpoint, :index, :en), %{ + "inserted_at" => DateTime.to_iso8601(inserted_at), + "hash" => Hash.to_string(hash) + }) - assert html_response(conn, 200) - assert 1 == Enum.count(conn.assigns.transactions) + actual_hashes = + conn.assigns.transactions + |> Enum.map(& &1.hash) + |> Enum.reverse() + + assert second_page_hashes == actual_hashes end - test "works when there are no transactions", %{conn: conn} do - conn = get(conn, pending_transaction_path(conn, :index, :en)) + test "next_page_params exist if not on last page", %{conn: conn} do + 60 + |> insert_list(:transaction) - assert html_response(conn, 200) + conn = get(conn, pending_transaction_path(ExplorerWeb.Endpoint, :index, :en)) + + assert conn.assigns.next_page_params + end + + test "next_page_params are empty if on last page", %{conn: conn} do + insert(:transaction) + + conn = get(conn, pending_transaction_path(ExplorerWeb.Endpoint, :index, :en)) + + refute conn.assigns.next_page_params end end end