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

Merge upstream releases v4.0.0, v4.0.1, and v4.0.2 #474

Merged

Conversation

aaronskiba
Copy link
Collaborator

@aaronskiba aaronskiba commented Oct 6, 2023

This is a link to a Google Sheets doc with very rough notes regarding how I resolved various conflicts while merging the three releases.

Fixes #470
Fixes #465
Fixes #430
Fixes #416
Fixes #414

benjaminfaure and others added 30 commits June 14, 2022 15:53
Usage: spring COMMAND [ARGS]

Commands for Spring itself:

  binstub         Generate Spring based binstubs. Use --all to generate a binstub for all known commands. Use --remove to revert.
  help            Print available commands.
  server          Explicitly start a Spring server in the foreground
  status          Show current status.
  stop            Stop all Spring processes for this project.

Commands for your application:

  rails           Run a rails command. The following sub commands will use Spring: console, runner, generate, destroy, test.
  rake            Runs the rake command
  rspec           Runs the rspec command to rebuild binstubs for the core rails components
…ers uniqueness to be case sensitive, so added case insensitivity flag to models and specs. Also added indexes for these
departments was broken.

DCC Issue https://github.com/DigitalCurationCentre/DMPonline-Service/issues/710

The fix involved making the following changes to the assign_users() and
unassign_users() methods in
app/controllers/api/v0/departments_controller.rb:
- Removed the previous redirect_to in each method and replaced it with
          render users_api_v0_departments_path
- Ensured the @users variable was set as follows
          @users = @user.org.users.includes(:department)
which is the way @users is set in the users() method in the controller.
The Json was called without this value during rendering.
Reverting a change from commit # 46eba76. This commit marks the merging of upstream release v4.0.2
@aaronskiba aaronskiba marked this pull request as ready for review October 23, 2023 21:35
This code was introduced when merging upstream release v4.0.0 (commit # 9421b57). Lines 86 and 92-96 were commented-out by me.
uat is the only config/environment that doesn't use Rails.application.routes.default_url_options[:host]
@@ -50,6 +51,10 @@ def as_csv(user,
end
end
end
csv << []
csv << []
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lines 54 and 55 are the same!

@@ -183,6 +214,21 @@ def prepare_coversheet_for_csv(csv, _headings, hash)
end
# rubocop:enable Metrics/MethodLength, Metrics/AbcSize

# rubocop:disable Metrics/AbcSize
def prepare_research_outputs_for_csv(csv, _headings, hash)
return false unless hash[:research_outputs].present? && hash[:research_outputs].any?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we directly call retrun unless instead of return false unless ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think here we want the false value explicitly instead of nil with just the return statement. This logic could be simplified though, we will be able to help more with this once we are more involved with core development.

csv << research_output.values
end
csv << []
csv << []
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need lines 227 and 228?

200455939-yashu
200455939-yashu previously approved these changes Nov 3, 2023
Copy link
Collaborator

@200455939-yashu 200455939-yashu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome Work 😇👏

Copy link
Collaborator

@lagoan lagoan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like good work! We should find time to go through the change together.

Gemfile Outdated
@@ -318,29 +326,29 @@ group :ci, :development do

# RuboCop is a Ruby code style checking and code formatting tool. It aims to enforce
# the community-driven Ruby Style Guide.
gem 'rubocop'
gem 'rubocop', '~> 1.44.1' # Pinned to align with upstream v4.0.2
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should add another comment on the line above pinning the gem specifying it is for dmp assistant

Comment on lines +336 to +351
# gem 'rubocop-performance'

# Automatic Rails code style checking tool. A RuboCop extension focused on enforcing
# Rails best practices and coding conventions.
gem 'rubocop-rails'
# gem 'rubocop-rails'

# A RuboCop plugin for Rake tasks
gem 'rubocop-rake'
# gem 'rubocop-rake'

# Code style checking for RSpec files. A plugin for the RuboCop code style enforcing
# & linting tool.
gem 'rubocop-rspec'
# gem 'rubocop-rspec'

# Thread-safety checks via static analysis. A plugin for the RuboCop code style
# enforcing & linting tool.
gem 'rubocop-thread_safety'
# gem 'rubocop-thread_safety'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aaronskiba do we know why these rubocop gems were commented out?

@@ -183,6 +214,21 @@ def prepare_coversheet_for_csv(csv, _headings, hash)
end
# rubocop:enable Metrics/MethodLength, Metrics/AbcSize

# rubocop:disable Metrics/AbcSize
def prepare_research_outputs_for_csv(csv, _headings, hash)
return false unless hash[:research_outputs].present? && hash[:research_outputs].any?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think here we want the false value explicitly instead of nil with just the return statement. This logic could be simplified though, we will be able to help more with this once we are more involved with core development.


# Used by Rails' routes url_helpers (typically when including a link in an email)
Rails.application.routes.default_url_options[:host] = "localhost:3000"
Rails.application.routes.default_url_options[:host] = Rails.application.secrets.dmproadmap_host
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We had a conversation before about the use of ENV variables. In this case, we could continue to use it since it is for the development environment, we use a lot of these variables at the moment. The production environment is where we handle it differently at this time.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aaronskiba Will add ENV variable backl

config/environments/production.rb Outdated Show resolved Hide resolved
config/environments/production.rb Outdated Show resolved Hide resolved
'require' statement is for the `config.active_record.database_selector = { delay: 2.seconds }` statement, which we are not using.
Commented lines 102-122 are for database switching, which we are also not using.
NOTE: CHANGELOG.md did not exist for upstream until release v4.0.2
Copy link
Collaborator

@lagoan lagoan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great progress @aaronskiba , this is a huge PR so there are only a few things to look at.

CHANGELOG.md Outdated
Comment on lines 3 to 35
## [Unreleased]

### Added

- Merged Upstream Release [v4.0.0](https://github.com/DMPRoadmap/roadmap/releases/tag/v4.0.0)

- Merged Upstream Release [v4.0.1](https://github.com/DMPRoadmap/roadmap/releases/tag/v4.0.1)

- Merged Upstream Release [v4.0.2](https://github.com/DMPRoadmap/roadmap/releases/tag/v4.0.2)
- #### [v4.0.2](https://github.com/DMPRoadmap/roadmap/releases/tag/v4.0.2)

- ##### Added
- Added CHANGELOG.md and Danger Github Action [#3257](https://github.com/DMPRoadmap/roadmap/issues/3257)
- Added validation with custom error message in research_output.rb to ensure a user does not enter a very large value as 'Anticipated file size'. [#3161](https://github.com/DMPRoadmap/roadmap/issues/3161)
- Added popover for org profile page and added explanation for public plan
- Added rack-attack version 6.6.1 gem. https://rubygems.org/gems/rack-attack/versions/6.6.1

- ##### Fixed
- Fixed an issue that was preventing uses from leaving the research output byte_size field blank
- Patched issue that was causing template visibility to default to organizationally visible after saving
- Froze mail gem version [#3254](https://github.com/DMPRoadmap/roadmap/issues/3254)
- Updated the CSV export so that it now includes research outputs
- Updated sans-serif font used in PDF downloads to Roboto since Google API no longer offers Helvetica
- Fixed discrepencies with default/max per_page values for API and UI pagination
- Updated JS that used to call the TinyMCE `setMode()` function so that it now calls `mode.set()` because the former is now deprecated.
- Patched an issue that was causing a template's visibility to change to 'organizationally_visible' when saving on the template details page.
- Fixed an issue with the Rails 6 keyword arguments change that was causing the `paginable_sort_link` to fail

- ##### Changed
- Added scss files to EditorConfig
- Change csv file name for statistics from 'Completed' to 'Created'
- Added error message and updated saving message for plan writing session to improve user experience

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For merging this file we can add the changes on the same indentation level as they are changes that come into that release. Nice idea of keeping the merged versions, we should keep that.

Comment on lines +133 to 136
<%= _('Organisation Types') %>
&nbsp; <a href="#" aria-label="<%= _('Text') %>" data-toggle="tooltip" data-placement="right" title="<%= org_types_tooltip %>">
<i class="fas fa-question-circle fa-reverse" ></i>
<i class="fas fa-question-circle fa-reverse" ></i>
<em class="sr-only"></em>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we were going to verify the tooltips were working. Did we verify this?

Keep upstream's changelog entries inline with our own.
Copy link
Collaborator

@lagoan lagoan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good job!

@aaronskiba aaronskiba merged commit 1fca7e6 into deployment-portage Dec 20, 2023
12 checks passed
@aaronskiba aaronskiba deleted the aaron/merge-upstream-releases-v4.0.0-v4.0.1-v4.0.2 branch December 20, 2023 17:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
9 participants