-
-
Notifications
You must be signed in to change notification settings - Fork 6.9k
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
Add search and sort functions to hashtag admin UI #11829
Add search and sort functions to hashtag admin UI #11829
Conversation
@@ -78,11 +74,14 @@ def filtered_tags | |||
scope = scope.unreviewed if filter_params[:review] == 'unreviewed' | |||
scope = scope.reviewed.order(reviewed_at: :desc) if filter_params[:review] == 'reviewed' | |||
scope = scope.pending_review.order(requested_review_at: :desc) if filter_params[:review] == 'pending_review' | |||
scope.order(max_score: :desc) | |||
scope = scope.matches_name(filter_params[:name].to_s.strip) unless filter_params[:name].nil? |
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.
Are there security concerns about the entered 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.
Seems fine to me.
@@ -131,6 +131,8 @@ en: | |||
must_be_follower: Block notifications from non-followers | |||
must_be_following: Block notifications from people you don't follow | |||
must_be_following_dm: Block direct messages from people you don't follow | |||
invite: | |||
comment: Comment |
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 translation has nothing to do with this PR.
However, this is necessary for translating invitation link comments.
I would like to include this translation in this PR if possible.
|
||
.actions | ||
%button= t('admin.accounts.search') | ||
= link_to t('admin.accounts.reset'), admin_tags_path, class: 'button negative' |
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.
These borrowed from existing translations.
Should I have these newly prepared?
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.
It's fine, we do that for other views with the same buttons.
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.
Looks fine to me overall, but other parts use a model to model the filters, e.g. the InstanceFilter
model. Maybe it would be more consistent to do it that way too?
@@ -78,11 +74,14 @@ def filtered_tags | |||
scope = scope.unreviewed if filter_params[:review] == 'unreviewed' | |||
scope = scope.reviewed.order(reviewed_at: :desc) if filter_params[:review] == 'reviewed' | |||
scope = scope.pending_review.order(requested_review_at: :desc) if filter_params[:review] == 'pending_review' | |||
scope.order(max_score: :desc) | |||
scope = scope.matches_name(filter_params[:name].to_s.strip) unless filter_params[:name].nil? |
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.
Seems fine to me.
|
||
.actions | ||
%button= t('admin.accounts.search') | ||
= link_to t('admin.accounts.reset'), admin_tags_path, class: 'button negative' |
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.
It's fine, we do that for other views with the same buttons.
Thank you very much! I will try to do that later. |
app/models/tag_filter.rb
Outdated
when 'order' | ||
scope_for_order(value) | ||
when 'review' | ||
scope_for_review(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.
I'm not sure if these method names are okay...
I'm also not sure if this is the best way to call these methods.
Is there a better idea?
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 seems OK.
app/models/tag_filter.rb
Outdated
def scope_for(key, value) | ||
case key.to_s | ||
when 'context' | ||
Tag.discoverable if value == 'directory' |
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.
What happens if scope.merge!(nil)
?
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.
Oh… thank you, I will fix it.
* Add search and sort functions to hashtag admin UI * Move scope processing from tags_controller to tag_filter * Fix based on method naming conventions * Fixed not to get 500 errors for invalid requests
Add search and sort features to the hashtag page of the Admin UI.
In the "ORDER BY" field "MOST RECENT", "MOST POPULAR", and "LAST ACTIVE" do not know if this name is best.
Also, is this order appropriate?
They are sorted for the following reasons:
MOST RECENT
Sorted in order from the newly received hashtag by the server
MOST POPULAR
Sort in order from biggest "max_score"
LAST ACTIVE
Sorted in order from newest "last_status_at"
I think I should write a test too.
However, there are a lot of things I don't understand yet and I haven't built a test environment.