-
Notifications
You must be signed in to change notification settings - Fork 24
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enable sort function by registered time on the search page #119
Enable sort function by registered time on the search page #119
Conversation
app/views/search/index.html.erb
Outdated
url_for(@search_request.to_params(order_target: "last_modified_at"))) %> | ||
</li> | ||
<li> | ||
<%= link_to_unless(@search_request.order_target == "registered_at", | ||
tag.i(class: "fas fa-clock") + " " + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use different icon?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix: 4e22729 Thanks. I changed the icon like the following.
Before | After |
---|---|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. They may be difficult to understand the difference of them...
Can we find a better icon to distinct them...?
https://fontawesome.com/search?q=clock&o=r&m=free
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix: dbcb629
I couldn't find suitable icons in the clock icon list and the other lists like history or date. I chose the following icons instead:
- Pen icon: Represents 'last_modified_at' as an edit.
- Calendar icon: Symbolizes 'registered_at' as a date-related event.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They will be better than the current icons.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
config/locales/en.yml
Outdated
label_full_text_search_order_target_last_modified_at: updated at | ||
label_full_text_search_order_target_registered_at: created at |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about using last modified time
and registered time
(or something)?
I'm not sure "XXX at" is a natural label for English speaker.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix: 21c083d
I have aligned the verbs in the past tense, referencing the services mentioned below.
Example 1 (GitHub) | Example 2(Google Drive) |
---|---|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or using updated
or created
simply might also be a good idea because some pages use the Created
or Updated
.
@@ -226,6 +226,7 @@ def output_columns | |||
"content_snippets", | |||
"id", | |||
"last_modified_at", | |||
"registered_at", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about showing this too in search result list?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kou
May I ask you the use-case about it? I guess we will use it the following place?
<span class="author"><%= format_time(e.event_datetime) %></span></dd> |
Do you mean we will include record.registered_at
in the following search result list?
If so, I'm thinking about how I should return it.
- Return the different fields
- Add the following fields to the response fields
api.last_modified_at: record.registered_at
api.registered_at: record.registered_at
- Add the following fields to the response fields
- Return it as the
api.datetime
- Change the value depending on the
order_target
field although I haven't had any image to implement it.- if order_target is registered_at,
api.datetime: record.registered_at
- if order_target is last_modified_at,
api.datetime: record.last_modified_at
- if order_target is registered_at,
- Change the value depending on the
redmine_full_text_search/app/views/search/index.api.rsb
Lines 4 to 14 in 3c14ef3
@result_set.each do |record| | |
api.result do | |
api.id record.event_id | |
api.title record.event_highlighted_title | |
api.type record.event_type | |
api.url url_for(record.event_url(:only_path => false)) | |
api.description record.event_content_snippets.join("\n") | |
api.datetime record.event_datetime | |
api.rank record.rank | |
end | |
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Return the different fields
- Add the following fields to the response fields
- api.last_modified_at: record.registered_at
- api.registered_at: record.registered_at
@kou
In this case, I expect it to be displayed on the screen as follows. What do you think of it?
Before | After |
---|---|
diff --git a/app/views/search/index.api.rsb b/app/views/search/index.api.rsb
index e61693c..40943cc 100644
--- a/app/views/search/index.api.rsb
+++ b/app/views/search/index.api.rsb
@@ -3,13 +3,15 @@ api.array :results, api_meta(:total_count => @result_set.n_hits,
:limit => @search_request.limit) do
@result_set.each do |record|
api.result do
- api.id record.event_id
- api.title record.event_highlighted_title
- api.type record.event_type
- api.url url_for(record.event_url(:only_path => false))
- api.description record.event_content_snippets.join("\n")
- api.datetime record.event_datetime
- api.rank record.rank
+ api.id record.event_id
+ api.title record.event_highlighted_title
+ api.type record.event_type
+ api.url url_for(record.event_url(:only_path => false))
+ api.description record.event_content_snippets.join("\n")
+ api.datetime record.event_datetime
+ api.last_modified_at record.last_modified_at
+ api.registered_at record.registered_at
+ api.rank record.rank
end
end
end
diff --git a/app/views/search/index.html.erb b/app/views/search/index.html.erb
index b9a46b3..3071fbe 100644
--- a/app/views/search/index.html.erb
+++ b/app/views/search/index.html.erb
@@ -189,7 +189,13 @@
<li><%= snippet %></li>
<% end %>
</ol>
- <span class="author"><%= format_time(e.event_datetime) %></span></dd>
+ <span class="last-modified">
+ <%= "#{l(:label_full_text_search_order_target_last_modified_time)}: #{format_time(e.last_modified_at)}" %>
+ </span>
+ <span class="registered">
+ <%= "#{l(:label_full_text_search_order_target_registered_time)}: #{format_time(e.registered_at)}" %>
+ </span>
+ </dd>
<% end %>
</dl>
(END)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just thought app/views/search/index.html.erb
but outputting in index.api.erb
too is better. Good catch.
I feel that it's difficult to distinct last_modified_at
value and registered_at
value because we don't have any separator between them.
How about using icon instead of l(:label_full_text_search_order_target_XXX)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix: 8ba3f0a Thanks!
@kou |
I'll re-review and reply after 2023-12-26... |
app/views/search/index.html.erb
Outdated
@@ -55,8 +55,12 @@ | |||
<%= l(:label_full_text_search_order_target_score) %> | |||
</label> | |||
<label> | |||
<%= form.radio_button "order_target", "date", name: "order_target" %> | |||
<%= l(:label_full_text_search_order_target_date) %> | |||
<%= form.radio_button "order_target", "last_modified_at", name: "order_target" %> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about using the same term for i18n label?
<%= form.radio_button "order_target", "last_modified_at", name: "order_target" %> | |
<%= form.radio_button "order_target", "last_modified_time", name: "order_target" %> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix: 177a642
app/views/search/index.html.erb
Outdated
url_for(@search_request.to_params(order_target: "last_modified_at"))) %> | ||
</li> | ||
<li> | ||
<%= link_to_unless(@search_request.order_target == "registered_at", | ||
tag.i(class: "fas fa-clock") + " " + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. They may be difficult to understand the difference of them...
Can we find a better icon to distinct them...?
https://fontawesome.com/search?q=clock&o=r&m=free
lib/full_text_search/searcher.rb
Outdated
@@ -242,10 +243,14 @@ def sort_keys | |||
direction = "" | |||
end | |||
case @request.order_target | |||
when "date" | |||
when "last_modified_at" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we keep supporting date
for a backward compatibility?
when "last_modified_at" | |
# TODO: Add a comment why "data" exists here | |
when "data", "last_modified_at" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix: 3063733
@@ -226,6 +226,7 @@ def output_columns | |||
"content_snippets", | |||
"id", | |||
"last_modified_at", | |||
"registered_at", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just thought app/views/search/index.html.erb
but outputting in index.api.erb
too is better. Good catch.
I feel that it's difficult to distinct last_modified_at
value and registered_at
value because we don't have any separator between them.
How about using icon instead of l(:label_full_text_search_order_target_XXX)
?
@kou |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know why CI isn't run for this PR...?
api.type record.event_type | ||
api.url url_for(record.event_url(:only_path => false)) | ||
api.description record.event_content_snippets.join("\n") | ||
api.datetime record.event_datetime |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a comment that this is deprecated but still exists for backward compatibility and using last_modified_at
or registered_at
explicitly is recommended?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix: 61f2096 Thanks!
app/views/search/index.html.erb
Outdated
tag.i(class: "fas fa-clock") + " " + | ||
l(:label_full_text_search_order_target_date), | ||
url_for(@search_request.to_params(order_target: "date"))) %> | ||
<%= link_to_unless(@search_request.order_target == "last_modified_time", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<%= link_to_unless(@search_request.order_target == "last_modified_time", | |
<%# TODO: Add a comment why "date" is here %> | |
<%= link_to_unless(@search_request.order_target.include?(["date", "last_modified_time"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix: 61f2096
Thanks, I missed it.
@@ -179,7 +189,15 @@ | |||
<li><%= snippet %></li> | |||
<% end %> | |||
</ol> | |||
<span class="author"><%= format_time(e.event_datetime) %></span></dd> | |||
<span class="last-modified"> | |||
<i class="fas fa-pen"></i>: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may not need :
here.
Could you share screenshot?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks.
We may need a separator by CSS (something like " 🖊️: 12/26/2023 05:50 PM | 📆: 12/26/2023 05:50 PM") between last modified time and registered time.
If we have a separator, I don't care with/without :
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix: 6f01fbb
Thank you for your suggestion. The visibility of content gets much better!
lib/full_text_search/searcher.rb
Outdated
@@ -242,10 +243,16 @@ def sort_keys | |||
direction = "" | |||
end | |||
case @request.order_target | |||
when "date" | |||
# TODO: Supporting the `date` parameter for backward compatibility, | |||
# but it will be deprecated in the future. Use `last_modified_time` instead. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can make it deprecated right now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix: 61f2096
if item.respond_to?(:created_on) | ||
datetime ||= item.created_on | ||
registered_at = item.created_on | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will simplify the following code (registered_at&.iso8601 || datetime&.iso8601
-> registered_at*.iso8601
):
end | |
else | |
registered_at = datetime | |
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix: f9fbeb0
@@ -215,13 +215,18 @@ def format_api_results(items, total_count: nil) | |||
datetime ||= changeset.committed_on | |||
end | |||
datetime ||= item.updated_on if item.respond_to?(:updated_on) | |||
datetime ||= item.created_on if item.respond_to?(:created_on) | |||
if item.respond_to?(:created_on) | |||
datetime ||= item.created_on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you rename the local varialble name to last_modified_at
from datetime
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix: 3bd7a5e
Ah, the workflow is disabled by scheduled workflow. |
Because the datetime(date) is deprecated now.
@kou |
app/views/search/index.api.rsb
Outdated
api.type record.event_type | ||
api.url url_for(record.event_url(:only_path => false)) | ||
api.description record.event_content_snippets.join("\n") | ||
# TODO: 'datetime' is deprecated but maintained for backward compatibility. Use 'last_modified_at' or 'registered_at' explicitly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you remove TODO
?
Could you fold a long line?
# TODO: 'datetime' is deprecated but maintained for backward compatibility. Use 'last_modified_at' or 'registered_at' explicitly. | |
# 'datetime' is deprecated but maintained for backward compatibility. | |
# Use 'last_modified_at' or 'registered_at' explicitly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix: 846e1ac
app/views/search/index.html.erb
Outdated
@@ -55,8 +55,14 @@ | |||
<%= l(:label_full_text_search_order_target_score) %> | |||
</label> | |||
<label> | |||
<%= form.radio_button "order_target", "date", name: "order_target" %> | |||
<%= l(:label_full_text_search_order_target_date) %> | |||
<!-- TODO: 'date' is deprecated but maintained for backward compatibility. --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you use ERB comment instead of HTML comment because this comment will be meaningless in rendered HTML?
<!-- TODO: 'date' is deprecated but maintained for backward compatibility. --> | |
<!-- 'date' is deprecated but maintained for backward compatibility. --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix: cbf714b
app/views/search/index.html.erb
Outdated
<%= l(:label_full_text_search_order_target_date) %> | ||
<!-- TODO: 'date' is deprecated but maintained for backward compatibility. --> | ||
<!-- Use 'last_modified_time' or 'registered_time' explicitly. --> | ||
<%= form.radio_button "order_target", "last_modified_time", checked: ["date", "last_modified_time"].include?(@search_request.order_target), name: "order_target" %> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you fold a long line for readability?
<%= form.radio_button "order_target", "last_modified_time", checked: ["date", "last_modified_time"].include?(@search_request.order_target), name: "order_target" %> | |
<%= form.radio_button "order_target", | |
"last_modified_time", | |
checked: ["date", "last_modified_time"].include?(@search_request.order_target), | |
name: "order_target" %> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix: afb81f1
@@ -179,7 +189,15 @@ | |||
<li><%= snippet %></li> | |||
<% end %> | |||
</ol> | |||
<span class="author"><%= format_time(e.event_datetime) %></span></dd> | |||
<span class="last-modified"> | |||
<i class="fas fa-pen"></i>: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks.
We may need a separator by CSS (something like " 🖊️: 12/26/2023 05:50 PM | 📆: 12/26/2023 05:50 PM") between last modified time and registered time.
If we have a separator, I don't care with/without :
.
Because this comment will be meaningless in rendered HTML
@kou |
assets/stylesheets/search.css
Outdated
@@ -82,6 +82,11 @@ ol.search-snippets li { | |||
padding: 0.25em 0; | |||
} | |||
|
|||
.last-modified::after { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you limit this style only in search result something like the following?
.last-modified::after { | |
#search-result .last-modified::after { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix: b66f956
@kou |
Could you update the PR title and description before we merge this? |
@kou |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, one more thing.
Could you add a test for order by registered time?
order_type: "desc", | ||
search_id: @project.id, | ||
news: "1", | ||
attachments: "0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't understand why the value of attachments parameter is '1' at first.
So if I don't use this parameter, the searched result includes targets related to attachments.
7fd2370
to
7703bb5
Compare
parameters = { | ||
order_target: "registered_time", | ||
order_type: "desc", | ||
search_id: @project.id, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this?
If we need this, is @project.id
a suitable value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix: cc5d0ec
I took request.project
for the search_id
parameter.
And it has already been defined in the following.
request.project = @project |
limit: "-1" | ||
} | ||
targets = search(parameters).records | ||
searched_news_ids = targets.collect(&:source_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about compare records instead of only record IDs?
If this test is failed, we can only know record IDs. But we want to know created_on
value for each record. If we use records instead of record IDs, failure message includes created_on
values too.
searched_news_ids = targets.collect(&:source_id) | |
searched_news = targets.collect(&:source_record) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix: a59d128
Thanks, I think I couldn't have this viewpoint when I wrote tests...
@@ -82,5 +82,41 @@ def test_syntax_error_query | |||
assert_equal([issue], | |||
targets.collect(&:source_record).sort_by(&:id)) | |||
end | |||
|
|||
def test_results_ordered_by_descending_registered_time |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we simplify this name?
def test_results_ordered_by_descending_registered_time | |
def test_order_registered_time_desc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix: 8fcf70f
It's better.
Yes. |
If we use records instead of record IDs, failure message show instance information which includes created_on values too.
@kou
Added: I created the issue about it |
Thanks! |
GitHub: fix #115
Related PR: #118
What I did
date
parameter and ensured backward compatibility in the processregistered_time
orlast_modified_time
instead ofdate
parameterWhat I checked
I checked whether the pgroonga command include
registered_at
in sort_key or not. And it does.`'slices[type_filtered].sort_keys', '-registered_at'`
News
by using registered time and check the logs rails generated.