Redmine

Nice feature, I've only one question: Why do you rescue the templates (rescue nil)?

Marius BALTEANU wrote:

Nice feature, I've only one question: Why do you rescue the templates (rescue nil)?

For this feature, I thought there was no information that should be displayed using the sidebar partial.
So I added `rescue nil` to make the template work with or without the sidebar partial.

Takenori TAKAKI wrote:

Marius BALTEANU wrote:

Nice feature, I've only one question: Why do you rescue the templates (rescue nil)?

For this feature, I thought there was no information that should be displayed using the sidebar partial.
So I added `rescue nil` to make the template work with or without the sidebar partial.

How about avoiding catching all exceptions by checking the existence of the partial with ActionView::LookupContext::ViewPaths#exists? ?

I have confirmed that the following experimental code works as expected. Perhaps we can define a helper method like partial_exists? or render_if_exists.

diff --git a/app/views/imports/new.html.erb b/app/views/imports/new.html.erb
index e7ca82428..4e9b89ffb 100644
--- a/app/views/imports/new.html.erb
+++ b/app/views/imports/new.html.erb
@@ -12,4 +12,4 @@
   <p><%= submit_tag l(:label_next).html_safe + " &#187;".html_safe, :name => nil %></p>
 <% end %>

-<%= render :partial => "#{import_partial_prefix}_sidebar" %>
+<%= render :partial => "#{import_partial_prefix}_sidebar" if lookup_context.exists?("#{import_partial_prefix}_sidebar", lookup_context.prefixes, true) %>

The render_if_exists method is much better than rescue nil, but to be honest, I don't see a real value for users to show the issues/time entries sidebar during import wizard, especially after we removed the navigations links from the sidebar (#30294, #315980). My recommendation is just to remove the sidebar, it's simplify the code.

But if we want to keep the sidebar, then let's render the administration sidebar in order to be consistent with issues/time entries.

Also, the new route added by the patch should be covered by a test in test/integration/routing/imports_test.rb.

Regarding the functionality, I'm wondering If we should support "Generate password" and "Send information to users". From a security/privacy point of view, I think it will nice to import users without defining a default password for them (even if you are an administrator). This can be added later if we agree.

Marius, thank you for your feedback.

Marius BALTEANU wrote:

The render_if_exists method is much better than rescue nil, but to be honest, I don't see a real value for users to show the issues/time entries sidebar during import wizard, especially after we removed the navigations links from the sidebar (#30294, #315980). My recommendation is just to remove the sidebar, it's simplify the code.

I agree with Marius on the value of the sidebar.

But if we want to keep the sidebar, then let's render the administration sidebar in order to be consistent with issues/time entries.

I use the 'layout/admin' on the patch because this feature is for admins. Therefore, the current patch already renders the administration sidebar.

Takenori TAKAKI wrote:

I agree with Marius on the value of the sidebar.

Let's wait for Go Maeda feedback on this.

I use the 'layout/admin' on the patch because this feature is for admins. Therefore, the current patch already renders the administration sidebar.

You're right, sorry for the mistake.

Marius BALTEANU wrote:

The render_if_exists method is much better than rescue nil, but to be honest, I don't see a real value for users to show the issues/time entries sidebar during import wizard, especially after we removed the navigations links from the sidebar (#30294, #315980). My recommendation is just to remove the sidebar, it's simplify the code.

I agree that it is hard to imagine the situation that users need the content in the sidebar while using the CSV Import Wizard. However, I am not sure if we can remove the sidebar because some third-party plugins may suppose the existence of sidebar or use hooks such as view_issues_sidebar_planning_bottom and view_issues_sidebar_queries_bottom.

Go MAEDA wrote:

I agree that it is hard to imagine the situation that users need the content in the sidebar while using the CSV Import Wizard. However, I am not sure if we can remove the sidebar because some third-party plugins may suppose the existence of sidebar or use hooks such as view_issues_sidebar_planning_bottom and view_issues_sidebar_queries_bottom.

I understand that there is a need to discuss the deletion of the sidebar during import wizard. On the other hand, I thought it would be good to discuss separately from the feature addition(Import user accounts from CSV file).

Go Maeda, what do you think?

Takenori TAKAKI wrote:

Go MAEDA wrote:

I agree that it is hard to imagine the situation that users need the content in the sidebar while using the CSV Import Wizard. However, I am not sure if we can remove the sidebar because some third-party plugins may suppose the existence of sidebar or use hooks such as view_issues_sidebar_planning_bottom and view_issues_sidebar_queries_bottom.

I understand that there is a need to discuss the deletion of the sidebar during import wizard. On the other hand, I thought it would be good to discuss separately from the feature addition(Import user accounts from CSV file).

Go Maeda, what do you think?

You're right, let's move the discussion about the sidebar in another ticket.

Regarding your patch, it looks good to me, one minor change that we can do is to move the new method to ApplicationHelper because the method is quite general and it can be used in other pages as well.

Thank you for making the patch even more sophisticated. I think this patch should be merged into the core.