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.

Sign up for GitHub

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

Jump to bottom

feat(Admin): Add links to download CSV in EN and FR #374

Merged
merged 1 commit into from
Jan 18, 2023
Merged

feat(Admin): Add links to download CSV in EN and FR #374

merged 1 commit into from
Jan 18, 2023

Conversation

santostiago
Copy link
Contributor

Task

Pull request checklist

Please check if your PR fulfills the following requirements:

  • Make sure you are requesting to pull a topic/feature/bugfix branch (right side). Don't request your master!
  • Make sure you are making a pull request against the develop branch (left side). Also you should start your branch off our develop.
  • Check the commit's or even all commits' message styles matches our requested structure.
  • Check your code additions will fail neither code linting checks nor unit test.

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

Issue Number: 181285671

What is the new behavior?

Does this introduce a breaking change?

  • Yes
  • No

Other information

@santostiago santostiago self-assigned this Nov 27, 2022
@santostiago
Copy link
Contributor Author

@tsubik I finished this one late and didn't check if all the specs are passing.
If they're failing, I'll fix them tomorrow, but you can check the PR nevertheless

tsubik
tsubik reviewed Nov 28, 2022
View reviewed changes
app/controllers/concerns/translatable.rb Outdated
@@ -0,0 +1,25 @@
# frozen_string_literal: true

module Translatable
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not a fan of this as we're moving away from the default way of adding extra includes in Active Admin which is overriding scoped_collection method. If I want to add extra includes I would not know what translate method stands for I would probably just add scoped_collection as in AA docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tsubik what you're suggesting is not to have the scoped_collection there?
It's a bunch of repeated code in most AA resources.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, I think more code but easier to undestand could be beneficial, as when someone looks at scoped_collection then it's obvious what it does as this is default mechanism of AA. In comparison API translate([[operator: :translations], [fmu: :translations]], false) is very criptic.

Copy link
Collaborator

@tsubik tsubik Nov 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I'm just not sure if it's worth it to DRY it, as it is making this more complex and harder to customize scoped collection later, let's say we want to sideload different relations if it's CSV download vs HTML page as I did many times.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also includes could be optimized further to only load I18n.locale.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually, it could be not worth it to try sideload only I18n.locale translations with custom where or some joins.

tsubik
tsubik reviewed Nov 28, 2022
View reviewed changes
app/controllers/concerns/translatable.rb Outdated
controller do
before_action :set_locale, only: :index

def set_locale
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 could move setting this before_action to active admin initializers as this only works when you provide locale param explicit so we can make this work for all resources, and I would keep scoped_collection as there were before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mmmm.. let me check it

tsubik
tsubik reviewed Nov 29, 2022
View reviewed changes
config/initializers/active_admin.rb
@@ -307,3 +292,21 @@
# Add Filters Extensions
ActiveAdmin::BaseController.include ActiveAdmin::FilterSaver::Controller
end

# Modifies the display of the CSVs so that it shows on to download an English and a French versions.
module ActiveAdmin::ViewHelpers::DownloadFormatLinksHelper
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice 👍

@santostiago santostiago force-pushed the feat-admin_download_multi_language branch 7 times, most recently from 3a9003d to 1b1db05 Compare January 8, 2023 17:49
@santostiago
Copy link
Contributor Author

@tsubik I changed it no to have the concern.

@santostiago santostiago marked this pull request as ready for review January 8, 2023 17:50
@tsubik
Copy link
Collaborator

tsubik commented Jan 9, 2023

Great. All look good except a couple of failing specs. I don't know why those are failing (checked them briefly). Please make sure all tests are passing and we can merge this.

@santostiago
Copy link
Contributor Author

It was late and I forgot to check it. I'll do it now.

@santostiago santostiago force-pushed the feat-admin_download_multi_language branch from 1b1db05 to 79d010f Compare January 15, 2023 20:40
@santostiago
Copy link
Contributor Author

The specs are failing because of the batch actions in observations.
Merging the PR that changes the batch actions for observations should fix it. If not, I'll check tomorrow why it's happening.

tsubik
tsubik approved these changes Jan 16, 2023
View reviewed changes
Copy link
Collaborator

@tsubik tsubik left a comment

Choose a reason for hiding this comment

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

Alright, let's wait for other PR to be merged and see if all specs are passing then this is good to merge 👍

@santostiago santostiago force-pushed the feat-admin_download_multi_language branch from 79d010f to d41c52e Compare January 17, 2023 19:30
@santostiago
Copy link
Contributor Author

Rebased

@santostiago
Copy link
Contributor Author

It still has failing specs. I'm going to check it.

@santostiago santostiago force-pushed the feat-admin_download_multi_language branch from d41c52e to 7e310c6 Compare January 17, 2023 21:29
@santostiago
Copy link
Contributor Author

Ok, it was because the observations didn't have translations and didn't load in the AA page.

@tsubik tsubik merged commit 7fe9965 into develop Jan 18, 2023
@tsubik tsubik deleted the feat-admin_download_multi_language branch January 18, 2023 12:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tsubik tsubik tsubik approved these changes

Assignees

@santostiago santostiago

Labels
Code Review Requested
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@santostiago @tsubik