AO3-5259 Deduplicate and order admin post tags

[neuroalien]

Pull Request Checklist

• Have you read "How to write the perfect pull request"?
• Have you read the contributing guidelines?
• Have you added tests for any changed functionality?
• Have you added the Jira issue number
  as the first thing in your pull request title (e.g. AO3-1234 Fix thing)
• Do you have fewer than 5 pull requests already open? If not, please wait
  until they are reviewed and merged before creating new pull requests.

Issue

https://otwarchive.atlassian.net/browse/AO3-5259

Purpose

What does this PR do?

Deduplicates and orders by name the admin post tags for admin posts filtered by language.

Note: I couldn't face writing a cucumber step for "and the tag is present exactly once". I welcome ideas on how to test, whether a whole integration test even adds more value than length to the testing suite, etc. I am trying to be really restrained and not rewrite the entire index action to use a presenter, façade, or some other kind of service object that is easier to test. >.>

Testing Instructions

How can the Archive's QA team verify that this is working as you intended?

On Jira.

References

Are there other relevant issues/pull requests/mailing list discussions?

Credit

What name and pronouns should we use to credit you in the Archive of Our Own's Release Notes?

alien, they/them

[ceithir]

Note: I couldn't face writing a cucumber step for "and the tag is present exactly once". I welcome ideas on how to test, whether a whole integration test even adds more value than length to the testing suite, etc. I am trying to be really restrained and not rewrite the entire index action to use a presenter, façade, or some other kind of service object that is easier to test. >.>

Would a controller test with a expect(assigns[:tags]).to be more manageable?

[neuroalien]

Note: I couldn't face writing a cucumber step for "and the tag is present exactly once". I welcome ideas on how to test, whether a whole integration test even adds more value than length to the testing suite, etc. I am trying to be really restrained and not rewrite the entire index action to use a presenter, façade, or some other kind of service object that is easier to test. >.>

Would a controller test with a expect(assigns[:tags]).to be more manageable?

A controller test would be more manageable, but I've been told that controller specs are considered deprecated and we shouldn't write new ones 😬 So I'm torn between adding more controller specs (because we already have them) and not adding them (because we don't want them to proliferate).

[ceithir]

While the question of controller tests is a hot potato within the Rails community, there's no current plan to move away from them for the archive, and we actually keep adding new ones regularly (cf).

So go as crazy with them as you want if that's for the greater good.

[neuroalien]

While the question of controller tests is a hot potato within the Rails community, there's no current plan to move away from them for the archive, and we actually keep adding new ones regularly (cf).

So go as crazy with them as you want if that's for the greater good.

Thank you for checking! I've produced a controller spec that replicated the bug, and then greened it. I've only tested the specific scenario in the issue because we already test the rest in cucumbers.

[brianjaustin]

A couple questions, and also posting the explain differences for my own reference

existing explain

EXPLAIN for: SELECT `admin_post_tags`.* FROM `admin_post_tags` INNER JOIN `admin_post_taggings` ON `admin_post_taggings`.`admin_post_tag_id` = `admin_post_tags`.`id` INNER JOIN `admin_posts` ON `admin_posts`.`id` = `admin_post_taggings`.`admin_post_id` WHERE `admin_posts`.`language_id` = 1
+----+-------------+---------------------+--------+--------------------------------------------+--------------------------------------------+---------+-------------------------------------------------------+------+-------------+
| id | select_type | table               | type   | possible_keys                              | key                                        | key_len | ref                                                   | rows | Extra       |
+----+-------------+---------------------+--------+--------------------------------------------+--------------------------------------------+---------+-------------------------------------------------------+------+-------------+
|  1 | SIMPLE      | admin_posts         | ALL    | PRIMARY                                    | NULL                                       | NULL    | NULL                                                  | 480  | Using where |
|  1 | SIMPLE      | admin_post_taggings | ref    | index_admin_post_taggings_on_admin_post_id | index_admin_post_taggings_on_admin_post_id | 5       | otwarchive_test.admin_posts.id                        | 1    | Using where |
|  1 | SIMPLE      | admin_post_tags     | eq_ref | PRIMARY                                    | PRIMARY                                    | 4       | otwarchive_test.admin_post_taggings.admin_post_tag_id | 1    |             |
+----+-------------+---------------------+--------+--------------------------------------------+--------------------------------------------+---------+-------------------------------------------------------+------+-------------+
3 rows in set (0.00 sec)

changed explain

EXPLAIN for: SELECT DISTINCT `admin_post_tags`.* FROM `admin_post_tags` INNER JOIN `admin_post_taggings` ON `admin_post_taggings`.`admin_post_tag_id` = `admin_post_tags`.`id` INNER JOIN `admin_posts` ON `admin_posts`.`id` = `admin_post_taggings`.`admin_post_id` WHERE `admin_posts`.`language_id` = 1 ORDER BY `admin_post_tags`.`name` ASC
+----+-------------+---------------------+--------+--------------------------------------------+--------------------------------------------+---------+-------------------------------------------------------+------+----------------------------------------------+
| id | select_type | table               | type   | possible_keys                              | key                                        | key_len | ref                                                   | rows | Extra                                        |
+----+-------------+---------------------+--------+--------------------------------------------+--------------------------------------------+---------+-------------------------------------------------------+------+----------------------------------------------+
|  1 | SIMPLE      | admin_posts         | ALL    | PRIMARY                                    | NULL                                       | NULL    | NULL                                                  | 480  | Using where; Using temporary; Using filesort |
|  1 | SIMPLE      | admin_post_taggings | ref    | index_admin_post_taggings_on_admin_post_id | index_admin_post_taggings_on_admin_post_id | 5       | otwarchive_test.admin_posts.id                        | 1    | Using where                                  |
|  1 | SIMPLE      | admin_post_tags     | eq_ref | PRIMARY                                    | PRIMARY                                    | 4       | otwarchive_test.admin_post_taggings.admin_post_tag_id | 1    |                                              |
+----+-------------+---------------------+--------+--------------------------------------------+--------------------------------------------+---------+-------------------------------------------------------+------+----------------------------------------------+
3 rows in set (0.00 sec)

[brianjaustin]

I know the issue specifically mentions translated posts, but should the same distinct and order logic apply here as well?

[sarken]

Admin post tag names have a uniqueness validation on them, so I'm not sure if distinct would add anything here. Adding a joins(:admin_posts) might not be a terrible idea to get rid of zero-use tags, but it is wandering a bit away from the original issue.

[brianjaustin]

It looks like we tend to use to eq more often than to eql. Is there a specific reason to use the latter here?

[neuroalien]

Thanks for reviewing! Could you clarify which are the things needing coder action, and which ones are optional?

[brianjaustin]

Ah sorry, I read over what Sarken said again and realised I slightly misinterpreted it. I think this should be good to go!