Vote #80729
完了Import user accounts from CSV file
0%
説明
It is a time-consuming task to add dozens of user accounts when launching a new Redmine server.
I think it can be resolved if Redmine has a feature to import user accounts from a CSV file. I often get requests for this feature from my customers.
journals
I made a patch to achive this feature in the same way as Importing Issue, Importing TimeEntry.
Admin users will be able to import the following information from CSV.
* Login
* Firstname, Lastname
* Email
* Language (e.g. 'en')
* Administrator (Yes / No)
* Authentication mode
* Password
* Must change password at next logon (Yes / No)
* Status (active / registered / locked)
* Custom Fields
--------------------------------------------------------------------------------
--------------------------------------------------------------------------------
Setting the target version to 4.2.0.
--------------------------------------------------------------------------------
Thank you for the patch but imports_test.rb fails after applying the patch. Could you look into this?
<pre>
Failure:
RoutingImportsTest#test_imports [/Users/maeda/redmines/trunk/test/test_helper.rb:313]:
The generated path </issues/imports/new> did not match </users/imports/new>.
Expected: "/users/imports/new"
Actual: "/issues/imports/new"
bin/rails test test/integration/routing/imports_test.rb:23
</pre>
--------------------------------------------------------------------------------
Thank you, Mr. Maeda, for pointing out the bug.
The test code I tried during development was still in the patch...
I will re-post a patch that removes the unnecessary test code.
--------------------------------------------------------------------------------
--------------------------------------------------------------------------------
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?":https://api.rubyonrails.org/classes/ActionView/LookupContext/ViewPaths.html#method-i-exists-3F ?
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@.
<pre><code class="diff">
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 + " »".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) %>
</code></pre>
--------------------------------------------------------------------------------
Go MAEDA wrote:
> How about avoiding catching all exceptions by checking the existence of the partial with "ActionView::LookupContext::ViewPaths#exists?":https://api.rubyonrails.org/classes/ActionView/LookupContext/ViewPaths.html#method-i-exists-3F ?
>
> 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@.
>
I think the proposed code is a more polite implementation.
I created a patch that defines ImportsHelper#render_if_exists, how do you like it?
--------------------------------------------------------------------------------
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.
--------------------------------------------------------------------------------
Marius BALTEANU wrote:
> You're right, let's move the discussion about the sidebar in another ticket.
>
Thanks for your comment and agreement!
> 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.
I think it's better that way.
I made a patch that moved the new method to ApplicationHelper.
--------------------------------------------------------------------------------
Thank you for making the patch even more sophisticated. I think this patch should be merged into the core.
--------------------------------------------------------------------------------
Committed the patch. Thank you for writing the patch for this feature.
--------------------------------------------------------------------------------
@user_import_test.rb@ randomly fails due to missing fixtures. It can be fixed with the following patch.
<pre><code class="diff">
diff --git a/test/unit/user_import_test.rb b/test/unit/user_import_test.rb
index fe1201dff..b790aeb98 100644
--- a/test/unit/user_import_test.rb
+++ b/test/unit/user_import_test.rb
@@ -20,6 +20,8 @@
require File.expand_path('../../test_helper', __FILE__)
class UserImportTest < ActiveSupport::TestCase
+ fixtures :users, :auth_sources, :custom_fields
+
include Redmine::I18n
def setup
</code></pre>
--------------------------------------------------------------------------------
Go MAEDA wrote:
> @user_import_test.rb@ randomly fails due to missing fixtures. It can be fixed with the following patch.
Committed the fix in r19802.
--------------------------------------------------------------------------------