This way we write the tests from the user's point of view: users can see
(for example) a proposal with the title "Make everything awesome", but
they don't see a proposal with a certain ID.
There are probably dozens, if not hundreds, of places where we could
write tests this way. However, it's very hard to filter which ones are
safe to edit, since not many of them have an HTML class we can use in
the tests, and adding a class might generate conflicts with CSS styles.
So, for now, I'm only changing the ones allowing us to cleanly remove
useless assignements while maintaining the code vertically aligned.
This is one of the most strange behaviours in ruby: if a variable
doesn't exist, assigning to itself will return `nil`.
So a line like:
mdmkdfm = ooops if mdmkdfm.respond_to?(:uiqpior)
Surprisingly will not raise any errors: the nonexistent `mdmkdfm`
variable will be evaluated to `nil`, `mdmkdfm.respond_to?(:uiqpior)`
will evaluate to `nil.respond_to?(:uiqpior)`, which will return `false`,
and then the line will be evaluated as `mdmkdfm = ooops if false`, which
will return `nil`.
Maybe in the future Ruby will change this behaviour. We hope CONSUL is
now in better shape if that ever happens :).
Joining two scopes with `+` does not remove duplicate records. Luckily
now that we've upgraded to Rails 5, we can join scopes using `.or`.
The test was testing for the presence of elements, bud didn't test for
duplicate records. Testing the exact contents of the array revealed this
behaviour.
When `valuator_group` was `nil`, `[valuator_group&.investment_ids]` is
evaluated to `nil`, and so we were adding an extra element to the array.
We could add a `compact` call to the resulting array, but I find it
easier to convert `nil` to an array using `to_a`.
When creating a budget investment with an unverified manager (for
example, a manager who isn't part of the local census), there's a
request to `Budgets::InvestmentsController#suggest`. Since the manager
isn't verified, suggestions can't be obtained.
There are serveral ways to fix this problem:
* Add a `suggest` action to Management::Budgets::InvestmentsController,
doing the same thing the main `suggest` action does.
* Give unverified users permission to access investment suggestions
* Give managers permission to access investment suggestions
I've chosen the last one because I thought it was simple and only
changed existing behaviour for managers, but any other solution would be
as valid. I haven't added the `phase: "accepting"` condition to keep it
simple, since a read-only action like this one in the management portal
isn't gonna create security risks.
When we were inserting a row or replacing an existing one (just like we
do when we click the link to select an investment), we were entering a
row containing all columns, and all of them were displayed even if they
had been excluded using the column selector.
This caused the table to move in a strange way, which sometimes made the
investment selection tests fail.
Using the same variable name makes the code more difficult to read.
We're also enabling the `no-param-reassing` rule since we accidentally
reassigned params in commit 824dd26d, and this rule will prevent us from
doing so in the future.
We originally wrote `undefined` in CoffeeScript.
CoffeeScript compiled it to `void 0` when generating the JavaScript
files. However, the reason to do so was the `undefined` variable was
mutable before ECMAScript 5. Using `undefined` is more intuitive, and
nowadays there's no reason to use `void 0`.
For now we're only adding rules related to spacing and double quotes,
following the same rules we use in Ruby, which are the same rules
CoffeeScript followed when compiling these files.
We're also using the recommended ESLint rules, which will warn us about
many JavaScript common pitfalls, the `strict` rule which enforces using
strict mode, and the `no-console` rule, which will prevent us from
shipping code meant for debugging.
Although it's arguably more common to use the JSON format to define
these rules, I've chosen YAML because it's the format we use in all our
linters.
These statements were automatically added by CoffeeScript.
I'm only removing the obvious cases; there might be more cases where the
`return` statement isn't necessary.
We accidentally removed the `count` option in commit 55fb14ac, which
made the translation return a hash.
The test is a bit hacky, which makes me think changing the user
interface would probably be a better solution.
Local variables are one of the things CoffeeScript doesn't compile to
modern JavaScript automatically: it uses `var` instead of `const` or
`let`.
Besides, using `$this = $(this)` is usually done to reference the
current object in another function where the current object is a
different one. Here we were using it with no clear purpose.
In JavaScript we cannot easily repeat something N times, and
CoffeeScript's range loop compiles into complex JavaScript.
We could also use `Array.from({ length: N })`, but that's not supported
by old browsers like Internet Explorer 11.
Iterating through the jQuery elements with `each` is more similar to
other iterators we use. Furthermore, we avoid declaring the `index`
variable we don't use.
The `append` method can handle arrays, so there's no need to loop
through each element.
There's more to improve on this method, like the `asc` variable being
global to a table instead of depending on the current column, but for
now I'm only refactoring and maintaining the current behaviour.
