Skip to content
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

Order legislation process tags alphabetically #3969

Merged
merged 1 commit into from
Apr 10, 2020

Conversation

javierm
Copy link
Member

@javierm javierm commented Apr 10, 2020

References

Background

The method tag_list_on doesn't add an ORDER_BY clause to the SQL query it generates, and so results may come in any order.

However, in the tests we were assuming the tags were ordered by ID in descending order. Since that isn't always the case, the tests were failing sometimes. The user experience was also not that great since tags could be ordered differently after reloading the page.

Objectives

Make sure legislation process tags are always ordered the same way.

Notes

One test failed with this message:

Failures:
  1) Admin collaborative legislation Update Change proposal categories
     Failure/Error: expect(page).to have_field("Categories", with: "bicycles, recycling")
       expected to find visible field "Categories" that is not disabled with value "bicycles, recycling" but there were no matches. Also found "", which matched the selector but not all filters. Expected value to be "bicycles, recycling" but was "recycling, bicycles"
     # ./spec/features/admin/legislation/processes_spec.rb:251:in `block (3 levels) in <top (required)>'

We could also use the same order admins used when adding the tags:

@process.customs.order("taggings.created_at").pluck(:name).join(", ")

However, I'm not sure it improves the user experience, and it makes the code more complicated.
benefit to administratos.

The method `tag_list_on` doesn't add an `ORDER_BY` clause to the SQL
query it generates, and so results may come in any order.

However, in the tests we were assuming the tags were ordered by ID in
descending order. Since that isn't always the case, the tests were
failing sometimes.

Ordering the tags alphabetically solves the problem. We could also use
the same order admins used when adding the tags:

```
@process.customs.order("taggings.created_at").pluck(:name).join(", ")
```

However, I'm not sure it improves the user experience, and it makes the
code more complicated.
benefit to administratos.
@javierm javierm added the Bug label Apr 10, 2020
@javierm javierm self-assigned this Apr 10, 2020
@javierm javierm merged commit 426c1c5 into master Apr 10, 2020
@javierm javierm deleted the fix_legislation_tag_order branch April 10, 2020 19:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant