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

Fix PostgreSQL GitHub Action and Tests For Rails 7 Upgrade #3435

Merged
merged 10 commits into from
Aug 19, 2024

Conversation

aaronskiba
Copy link
Contributor

@aaronskiba aaronskiba commented Jul 6, 2024

Fixes #3419

Changes proposed in this PR:

  • This PR seeks to fix the PostgreSQL GitHub Actions for PR Updated app to rails 7 #3426 (NOTE: Fix PostgreSQL GitHub Action and Tests For Rails 7 Upgrade #3435 (comment) explains why the MySQL GitHub Action is still not working)

    • The copying of credentials via config/credentials.yml.mysql2 and config/credentials.yml.postgresql had to be updated for the Rails 7 upgrade.
    • DISABLE_SPRING=1 has been added to the test db setup commands. Without disabling Spring, the test db was failing to build with the Rails 7 upgrade.
  • The Style/SymbolProc-related Rubocop fixes applied in commit bda5b6e have been undone. Although executing rubocop -A results in these changes, those changes were causing errors within the code. (To address these Rubocop offences, commit 283e585 has been added instead.)

  • This PR also fixes failing tests executed by the PostgreSQL GitHub Action. These changes include the following:

    • Modified Template.visibility output
    • Making the following change (within RSpec tests only):
    • # Old Code
      post api_v1_authenticate_path, params: @payload.to_json
      # New Code (use `as:` for encoding the request's content type)
      post api_v1_authenticate_path, params: @payload, as: :json
    • Commit 5d06d6b updates the initialisation code for "@rails/ujs". Many of the spec/features/ were failing without the now updated ES6 module syntax.
  • This PR also resolves the GitHub issue Template.visibililty Checks Not Always Working as Expected #3419.

    • This issue existed prior to the Rails 7 upgrade. However, the Rails 7 upgrade has modified the behaviour of Template.visibilty; thus, the issue is resolved here.

Addresses the following error that was encountered when attempting to execute the GitHub Actions that correspond with the edited workflows files:

https://github.com/DMPRoadmap/roadmap/actions/runs/9513143079/job/26222602325
3s

```
Run bundle exec rails db:create RAILS_ENV=test
To use retry middleware with Faraday v2.0+, install `faraday-retry` gem
To use multipart middleware with Faraday v2.0+, install `faraday-multipart` gem; note: this is used by the ManageGHES client for uploading licenses
Copying Bootstrap glyphicons to the public directory ...
Copying TinyMCE skins to the public directory ...
/home/runner/work/roadmap/roadmap/config/initializers/recaptcha.rb:8:in `block in <main>': undefined method `[]' for nil:NilClass (NoMethodError)
	from /home/runner/work/roadmap/roadmap/vendor/bundle/ruby/3.0.0/gems/recaptcha-5.17.0/lib/recaptcha.rb:37:in `configure'
	from /home/runner/work/roadmap/roadmap/config/initializers/recaptcha.rb:7:in `<main>'
```
Prior to this commit, the Rails credentials were not being updated during this setup process.
@aaronskiba aaronskiba marked this pull request as draft July 8, 2024 07:13
This commit undoes some Rubocop fixes made from a prior commit ( bda5b6e ). However, it also resolves the following error that was being raised: https://github.com/DMPRoadmap/roadmap/actions/runs/9845084297/job/27179836711
Copy link

github-actions bot commented Jul 8, 2024

</tr>
1 Error
🚫

Please include a CHANGELOG entry.

You can find it at [CHANGELOG.md](https://github.com/DMPRoadmap/roadmap/blob/main/CHANGELOG.md).

Generated by 🚫 Danger

@aaronskiba aaronskiba changed the title Edit "Setup Credentials" step within workflows Fix PostgreSQL and MySQL2 GitHub Actions For Rails 7 Upgrade Jul 8, 2024
@aaronskiba
Copy link
Contributor Author

aaronskiba commented Jul 9, 2024

The failing tests still need to be addressed for the PostgreSQL GitHub action, but the workflow is now executing.

The MySQL GitHub action is still failing to execute. Many errors like the following are being thrown:

ActiveRecord::MismatchedForeignKey: Column `org_id` on table `annotations` does not match column `id` on `orgs`, which has type `bigint unsigned`. To resolve this issue, change the type of the `org_id` column on `annotations` to be :bigint. (For example `t.bigint :org_id`).

These errors are related to the migrations added to #3426. When the migrations were run, many tables not targeted by the migrations were also updated. Here is an example:

# before running migrations
create_table "orgs", id: :integer, force: :cascade do |t|
# after running migrations
create_table "orgs", id: :serial, force: :cascade do |t|

It seems that :serial is interpreted differently by MySQL and PostgreSQL. I think PostgreSQL casts it to type :integer, whereas MySQL casts it to type :bigint. Because the associated foreign keys are defined as type :integer, This would explain why the MySQL GitHub Action is now failing.

I'm not sure how to proceed here. Should we simply retire the MySQL GitHub Actions? Or perhaps re-running the migrations with a MySQL db would re-cast the primary keys to type :integer (I can't seem to get ruby bin/setup mysql to work on my machine)?

Wondering what your thoughts might be here @benjaminfaure and @briri. Thank you.

Omitting the arguments results in lambda implicitly using self, which appears to be the desired behaviour here. It also resolves the Rubocop offences.
@aaronskiba
Copy link
Contributor Author

There are now two failing tests within the PostgreSQL workflow. Although the tests were passing prior to this Rails 7 upgrade, there were known issues prior to it:

Failures:

  1) Template#customize! sets visibility to Organisationally visible
     Failure/Error: expect(subject.visibility).to eql(Template.visibilities['organisationally_visible'])

       expected: 0
            got: "organisationally_visible"

       (compared using eql?)
     # ./spec/models/template_spec.rb:1086:in `block (3 levels) in <main>'

  2) Template#upgrade_customization! sets the visibility to Organisationally visible
     Failure/Error: expect(subject.visibility).to eql(Template.visibilities['organisationally_visible'])

       expected: 0
            got: "organisationally_visible"

       (compared using eql?)
     # ./spec/models/template_spec.rb:1155:in `block (3 levels) in <main>'

`template.visibilty` now returns a string rather than an integer.

The Rails 7 upgrade actually fixes a couple of bugs within `app/views/org_admin/templates/_form.html.erb` and `app/views/org_admin/templates/_show.html.erb`. Prior to this upgrade, template.visibility would return an integer. Now that it is returning a string, the `f.object.visibility == 'organisationally_visible'` and `template.visibility == 'organisationally_visible'` checks within the aforementioned files are behaving as desired.
Prior to this commit, the default checked/unchecked values were used (i.e. "1" would be returned when checked, and "0" would be returned when unchecked). However, the box is meant to be checked when selecting 'organisationally_visible' ('for internal %{org_name} use only'), which makes the default checked/unchecked values opposite to the mapping of our enums (i.e. `{"organisationally_visible"=>0, "publicly_visible"=>1}`).
@aaronskiba
Copy link
Contributor Author

aaronskiba commented Jul 10, 2024

There are now two failing tests within the PostgreSQL workflow. Although the tests were passing prior to this Rails 7 upgrade, there were known issues prior to it:

The aforementioned issues have been addressed, but now the following ./spec/requests/api/v1/authentication_controller_spec.rb tests are breaking:

Failures:
  1) Api::V1::AuthenticationController actions POST /api/v1/authenticate renders /api/v1/error template if authentication fails
     Failure/Error: expect(response.code).to eql('401')
       expected: #<Encoding:UTF-8> "401"
            got: #<Encoding:US-ASCII> "400"
       (compared using eql?)
     # ./spec/requests/api/v1/authentication_controller_spec.rb:31:in `block (4 levels) in <main>'
  2) Api::V1::AuthenticationController actions POST /api/v1/authenticate returns a JSON Web Token
     Failure/Error: expect(response.code).to eql('[200](https://github.com/DMPRoadmap/roadmap/actions/runs/9881513683/job/27292456367?pr=3435#step:13:201)')
       expected: #<Encoding:UTF-8> "200"
            got: #<Encoding:US-ASCII> "400"
       (compared using eql?)
     # ./spec/requests/api/v1/authentication_controller_spec.rb:39:in `block (4 levels) in <main>'

@aaronskiba
Copy link
Contributor Author

Failures:
  1) Api::V1::AuthenticationController actions POST /api/v1/authenticate renders /api/v1/error template if authentication fails
     Failure/Error: expect(response.code).to eql('401')
       expected: #<Encoding:UTF-8> "401"
            got: #<Encoding:US-ASCII> "400"
       (compared using eql?)
     # ./spec/requests/api/v1/authentication_controller_spec.rb:31:in `block (4 levels) in <main>'
  2) Api::V1::AuthenticationController actions POST /api/v1/authenticate returns a JSON Web Token
     Failure/Error: expect(response.code).to eql('[200](https://github.com/DMPRoadmap/roadmap/actions/runs/9881513683/job/27292456367?pr=3435#step:13:201)')
       expected: #<Encoding:UTF-8> "200"
            got: #<Encoding:US-ASCII> "400"
       (compared using eql?)
     # ./spec/requests/api/v1/authentication_controller_spec.rb:39:in `block (4 levels) in <main>'
   29:       # POST /api/v1/authenticate
   30:       # rubocop:disable Metrics/AbcSize
   31:       def authenticate
   32:         byebug
=> 33:         body = request.body.read
   34:         json = JSON.parse(body)
   35:         auth_svc = Api::V1::Auth::Jwt::AuthenticationService.new(json: json)
   36:         @token = auth_svc.call
   37: 
(byebug) request.headers['Content-Type']
"application/x-www-form-urlencoded"
(byebug) request.body.read
""

Rails 7 appears to apply stricter parsing rules. If the Content-Type is application/x-www-form-urlencoded, the body will not be parsed as JSON.

The following code changes throughout the codebase seem to resolve this:

# Old Code
post api_v1_authenticate_path, params: @payload.to_json
# New Code (use `as:` for encoding the request's content type)
post api_v1_authenticate_path, params: @payload, as: :json

@aaronskiba aaronskiba marked this pull request as ready for review July 15, 2024 19:42
@aaronskiba aaronskiba changed the title Fix PostgreSQL and MySQL2 GitHub Actions For Rails 7 Upgrade Fix PostgreSQL GitHub Actions For Rails 7 Upgrade Jul 15, 2024
Rails 7 appears to apply stricter parsing rules. If the Content-Type is not JSON, then the body will not be parsed as JSON.
Prior to this code change, any value assigned to the `'data-method':` attribute of the `link_to` method was not being read (and instead defaulting to `GET`). This was resulting in the breaking of several `spec/features/` tests (https://github.com/DMPRoadmap/roadmap/actions/runs/9946998559/job/27478725801). The `@rails/ujs` library is meant to handle this `'data-method':` attribute.
@aaronskiba aaronskiba changed the title Fix PostgreSQL GitHub Actions For Rails 7 Upgrade Fix PostgreSQL GitHub Action and Tests For Rails 7 Upgrade Jul 18, 2024
@aaronskiba
Copy link
Contributor Author

Hi @benjaminfaure, thanks for the approval on this PR. I added a couple more commits, so maybe it'd be best to get one more review/approval before I merge it into your Rails 7 upgrade branch? Thank you. :)

@aaronskiba aaronskiba merged commit d2a1eac into rails7 Aug 19, 2024
7 of 9 checks passed
@aaronskiba aaronskiba deleted the aaron/rails7-copy branch August 19, 2024 15:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants