We've been doing manually for too long ;).
The reason why we're assigning the author is it makes it easy to filter
pull requests by assignee on our kanban; it isn't so easy (actually,
might be impossible) to filter by author.
Without this condition, the kanban assignment would be run on every
CONSUL fork and that would result in unpredictable results since they
would try to write on our kanban, and they don't have permission to do
so.
Note that, much to my dismay :D, the code only works if we use single
quotes in the name of the repository owner; it doesn't work if we use
double quotes.
Rubocop was complaining about Layout/EmptyLinesAroundAccessModifier in
the tags controller.
This issue was introduced in commit e76735031. Unfortunately, it looks
like Pronto doesn't detect this issue because the access modifier was
already there; only the lines below it were introduced in that pull
request.
Rubocop was complaining about a Layout/ExtraSpacing in a couple of
places.
These issues weren't detected by Pronto because they didn't affect lines
changed in the pull request. These lines were fine until we removed the
lines next to them in commits 4b42a68b6 and 00f0c4410.
This test has been failing many times because it checks the database
after starting the process running the browser.
We're disabling JavaScript like we do in every other test using the
`create_proposal_notification` method. This way, Capybara will use the
rack driver and there'll be no risk of errors that currently might take
place if both the process running the browser and the process running the
test access the database.
This cop scans only the tests files by default, but we prefer to scan all
application Ruby files, so when a developer uses the class method
`I18n.locale=`, the cop will embrace using the method
`I18n.with_locale` instead. By doing this way, the cop will help
developers to avoid unexpected translation errors.
Quoting the Rails 6 guides:
> I18n.locale can leak into subsequent requests served by the same
thread/process if it is not consistently set in every controller. For
example executing I18n.locale = :es in one POST requests will have
effects for all later requests to controllers that don't set the locale,
but only in that particular thread/process. For that reason, instead of
I18n.locale = you can use I18n.with_locale which does not have this
leak issue.
Now we enabled the cop for all application Ruby files; we have to
remove the assignments at the controller level to set the request
locale. As Rails 6 guides suggest [1], we can use the `around_action`
controller callback to set each request locale without breaking the
rule.
This cop will warn CONSUL developers when using `I18n.locale`
assignment embracing them to use the `I18n.with_locale`instead.
[1] https://guides.rubyonrails.org/i18n.html#managing-the-locale-across-requests
So now, newly opened pull requests will automatically be added to the
"Reviewing" column, while newly opened draft pull requests will
automatically be added to the "Doing" column.
I've added the "reopened" event just in case (and was indeed very useful
while testing this feature), although we rarely reopen pull requests.
Note that this only works on pull requests that aren't already in the
project; that is, if a pull request is already in the "Doing" column,
closing it and reopening it will *not* move it to the "Reviewing
column".
So it looks like we won't easily be able to extend this feature in order
to automatically move pull requests when they're marked as ready for
review.
When we added this setting in commit 9979b5399, we disabled it by
default so it would be compatible with existing installations.
Since then, we've released version 1.4, which adds the settings to
existing databases. That means we can now enable it by default and
existing installations won't be affected.
When we perform database queries in tests after the process running the
browser has started, we sometimes get failures in our test suite due to
both the tests and the browser accessing the database at the same time.
Furthermore, using `Poll.all` results in a database query, and doing so
after the process running the browser has started might result in
failures when running our test suite.
We were clicking on the "Sign in" link right after clicking on the "Sign
out" link, which might result in simultaneous requests and exceptions
when running our test suite.
So we're adding an expectation to make sure the first request has
finished before starting the following one.
We were doing a `mappable.map_location` call in an `expect` which might
result in a database queries. Doing database queries in a test after the
process running the browser has started might result in exceptions while
running our test suite.
There were cases where we clicked the button to submit the form and
immediately we visited a different page.
In the past, we've had similar code produce PG::ProtocolViolation errors
in similar situations. Since we've had these errors a few times in the
nested imageable specs, there's a chance they're related to the absence
of the expectation.
Although I'm not even remotely sure this will fix these issues, at least
we now follow the convention of making expectations after every request.
Note we're changing both the nested imageable and nested documentable
specs. Only the nested imageable would need to be changed because it's
the one where there's a `visit` inside the
`imageable_redirected_to_resource_show_or_navigate_to` method. I'm
changing both for consistency.
We were missing a notice in this case. Not only this caused
inconsistencies in the user experience, but it also made it hard to add
an expectation in the test checking the request had finished before
making a new one. Simultaneous requests sometimes cause failures in our
test suite.
