Moderate legislation proposals
- added a controller for moderation/legislation
- updated view to appropriate link + added route
- added a spec
- Feature test
- test for faded
- javascripts for visual effects
These files create a fake class using an instance variable. While the
proper thing to do would be to refactor the `HasOrders` and `HasFilters`
concerns so they didn't use instance variables but methods, I don't
think that's going to happen in the near future.
This one is a bit different than our usual scenario, since we create
three annotations and we only use two of them in the specs (because we
visit the path to that annotation). So there are probably better options
than the combination of `let!` and `before` I've chosen.
Having two questions, each of them with two comments, made the code hard
to follow.
Grouping the comments inside the block creating the questions makes it
easier to know which comment belongs to which question, even if the code
is still not 100% readable.
We also remove instance variables, which by the way used the same
variable name for two different things.
We couldn't declare them inside the block because they would be
considered local variables and its value would be lost when the block
was finished. So we were using instance variables instead.
However, with instance variables we don't get any warnings when we
misspell their names. We can avoid them by declaring the local variables
before the block starts.
I haven't found an elegant way to remove them, but since they were the
only three variables left out of 383 we used to have, I can live with
this low percentage of inelegant solutions.
We had four headings, some of them had investments, and some of them
didn't, and it was very hard to scan the code and check which investment
belongs to which heading.
Grouping the investments inside the block creating the heading makes
that task much easier, even if the code is still not 100% readable.
We also avoid unused variables which were there to keep the code
vertically algined.
We can change the code a bit so the useless assignment is either part of
the setup (where only another variable was present) or isolated in the
"action" part of the test.
There's a very common pattern in our test, where the setup only has two
lines:
variable = create(:something)
unused_variable = create(:something_else, something: variable)
In this case, since there's a blank line below these ones and then we'll
get to the body of the test, and the second variable is going to be
created based on the first variable, we can remove the useless
assignment and the readability is still OK.
Another option we almost unanimously discarded was:
variable = create(:something)
_unused_variable = create(:something_else, something: variable)
We don't use it anywhere else, either.
One more option we considered but found a bit too much for simple tests:
variable = create(:something) do |something|
create(:something_else, something: variable)
end
Then of course we could move the setup to `let` and `before` blocks, but
the tests could get over-structured really quickly.
These variables can be considered a block, and so removing them doesn't
make the test much harder to undestand.
Sometimes these variables formed the setup, sometimes they formed an
isolated part of the setup, and sometimes they were the part of the test
that made the test different from other tests.
The limit parameter wasn't specified in the test but in the default
value in the database, making the test hard to read.
Since now we've moved the other processes to separate tests, now we can
create four processes using `times` and keep the test simple.
In the scenario where we want to test scopes and use `match_array`, we
usually declare variables we never use, which raises a warning in the
Ruby interpreter (since the main cause for an unused variable is a
typo).
So I've decided to just split the tests into cases where every record is
returned and cases were no records are returned, just like we do in
other places.
There are several other options we've considered:
1. Don't declare unused variables, but declare the ones we use
2. Prefix unused variables with un underscore
3. Declare just one variable being an array containing all elements, and
access the elements using Array#[]
4. Don't declare any variables, and compare results against attributes
such as titles
None of these options was met with enthusiasm.
The test was hard to follow, and splitting the test in three it's easier
to read and doesn't create unused variables anymore. On the minus side,
now there's one extra request during the tests.
