Skip to content
Snippets Groups Projects

Add a name field to the group edit form

Closed Add a name field to the group edit form
(removed):add-field-for-group-name into master
1 unresolved thread
Closed username-removed-1181022 requested to merge (removed):add-field-for-group-name into master
1 unresolved thread

What does this MR do?

Adds a text field to the group create and edit forms that enables edit of the group name. Formerly the value of the name for the new or existing group was copied from the value of the path.

I'm mentioning @awhildy and @hazelyang from the issue thread to bring it to your attention.

Thanks to @smcgivern and @ClemMakesApps for mentoring help.

Are there points in the code the reviewer needs to double check?

Ensure test coverage is sufficient and conforms to expectations.

Why was this MR needed?

This satisfies a feature request in the backlog for community contribution to the community edition.

Screenshots (if relevant)

GroupNew

GroupNewName

GroupEdit

Does this MR meet the acceptance criteria?

What are the relevant issue numbers?

25474

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • username-removed-443319
    username-removed-443319 @smcgivern ·
    Contributor

    Thanks @dclovell! I'll let @ClemMakesApps or another frontend MR coach take this, but I would say that we're moving away from Spinach tests towards RSpec tests, so any substantial new tests would ideally be in RSpec :thumbsup:

  • username-removed-443319 added ~164274 frontend labels

    added ~164274 frontend labels

  • username-removed-408677
    username-removed-408677 @ClemMakesApps ·
    Maintainer

    Yep. Feel free to do the tests in rspec or jasmine :smile:

    Thanks for contributing :tada:

  • username-removed-1181022 added 1 commit

    added 1 commit

    • bc684b69 - Cause new group form to copy name from path

    Compare with previous version

  • username-removed-1181022
    username-removed-1181022 @dclovell ·
    Author

    Tests are RSpec, added to spec/features/admin/admin_groups_spec.rb

  • username-removed-1181022 marked the task Conform by the style guides as completed

    marked the task Conform by the style guides as completed

  • username-removed-1181022 added 1 commit

    added 1 commit

    • 5c955909 - Remove duplicate group name entry field on group edit form

    Compare with previous version

  • username-removed-1181022 added 2 commits

    added 2 commits

    • b5b38dc8 - Address eslint errors in group.js
    • 91f066f9 - Enable group name copy with dispatch and explore group creation

    Compare with previous version

  • username-removed-1181022 added 2 commits

    added 2 commits

    • a1140a4d - Update workflow new group instruction
    • f6b008a8 - Update the gitlab basics group creation document

    Compare with previous version

  • username-removed-1181022 marked the task Documentation created/updated as completed

    marked the task Documentation created/updated as completed

  • username-removed-1181022 added 1 commit

    added 1 commit

    Compare with previous version

  • username-removed-1181022
    username-removed-1181022 @dclovell ·
    Author

    I have tried and failed to squash. Squashing commits is a new thing for me. @ClemMakesApps

    git rebase -i b4b56faaee and change a bunch of pick commands to squash, but when I tried to push, my HEAD was behind the remote.

  • username-removed-1181022 marked the task Changelog entry added as completed

    marked the task Changelog entry added as completed

  • username-removed-1181022 marked the task Conform by the merge request performance guides as completed

    marked the task Conform by the merge request performance guides as completed

  • username-removed-1181022 added 3 commits

    added 3 commits

    • c0149cc0 - Add a name field to the group edit form
    • 31395423 - Merge branch 'add-field-for-group-name' of gitlab.com:dclovell/gitlab-ce into ad…
    • 26e38cd2 - Remove unused variable for eslint

    Compare with previous version

  • username-removed-408677
    username-removed-408677 @ClemMakesApps ·
    Maintainer

    @dclovell you can squash your commits by doing git rebase -i HEAD~<# number of commits you want to squash>. In this case, you'd want to do git rebase -i HEAD~11 because you have 11 commits.

  • username-removed-1181022 added 374 commits

    added 374 commits

    • 26e38cd2...47a4b6e8 - 364 commits from branch gitlab-org:master
    • aab5d397 - Add a name field to the group edit form
    • 2b62e417 - Cause new group form to copy name from path
    • 77c0b3ec - Remove duplicate group name entry field on group edit form
    • c5dca821 - Address eslint errors in group.js
    • da4a8e99 - Enable group name copy with dispatch and explore group creation
    • 483868f1 - Update workflow new group instruction
    • 5c896829 - Update the gitlab basics group creation document
    • af6eb3e3 - Add a change log entry
    • 87c673ce - Remove unused variable for eslint
    • ab164e4e - Merge branch 'add-field-for-group-name' of gitlab.com:dclovell/gitlab-ce into ad…

    Compare with previous version

    Toggle commit list
  • username-removed-1181022
    username-removed-1181022 @dclovell ·
    Author

    My git version 2.12.0. While I understand conceptually what rebase does, I've always avoided it. Merge has kept me happy. so this is new.

    Thank you, @ClemMakesApps. I didn't have a problem selecting the commits for rebase/squash. Here is what I'm getting in painful detail so you might see where I'm going wrong. First, the log before starting. The branch is up to date with the remote (git pull). This is the top of the output from git log --oneline | head -n 14:

    26e38cd290 Remove unused variable for eslint
3139542392 Merge branch 'add-field-for-group-name' of gitlab.com:dclovell/gitlab-ce into add-field-for-group-name
c0149cc046 Add a name field to the group edit form
257b4c474c Add a change log entry
f6b008a8e7 Update the gitlab basics group creation document
a1140a4d3b Update workflow new group instruction
91f066f982 Enable group name copy with dispatch and explore group creation
b5b38dc84a Address eslint errors in group.js
5c9559092f Remove duplicate group name entry field on group edit form
bc684b69fa Cause new group form to copy name from path
a550adfd2f Add a name field to the group edit form
b4b56faaee Merge branch 'add-frequently-used-emojis-back-to-menu' into 'master'
c4ba6ea684 Merge branch 'fix-500-in-notes-polling' into 'master'
daa4590ca3 Merge branch '29263-merge-button-color' into 'master'

    The top eleven are mine. Now git rebase -i HEAD~11 gives this to edit:

    pick 543a68cfb5 Document gitaly.socket_path setting
pick cd4073ca1b Update feature visibility spec for projects edit page
pick c92808ed32 Fix for creating a project through API when import_url is nil
pick 16475bede7 Enable snippets for new projects by default
pick 5a135264ae adds queue option to push bulk in authorized projects worker
pick 3c53f66c7e Consistent button colors for disabled buttons
pick 394d83ff17 Fix notes polling failing after code changes
pick 3f5919e2c4 Add frequently used emojis back to awards menu
pick c0149cc046 Add a name field to the group edit form
pick a550adfd2f Add a name field to the group edit form
pick bc684b69fa Cause new group form to copy name from path
pick 5c9559092f Remove duplicate group name entry field on group edit form
pick b5b38dc84a Address eslint errors in group.js
pick 91f066f982 Enable group name copy with dispatch and explore group creation
pick a1140a4d3b Update workflow new group instruction
pick f6b008a8e7 Update the gitlab basics group creation document
pick 257b4c474c Add a change log entry
pick 26e38cd290 Remove unused variable for eslint

# Rebase dc99f343af..26e38cd290 onto dc99f343af (18 commands)

    Very strange. Those aren't my commits at all, until halfway down. I have tried deleting the commits I don't recognize, editing as follows:

    pick c0149cc046 Add a name field to the group edit form
squash a550adfd2f Add a name field to the group edit form
squash bc684b69fa Cause new group form to copy name from path
squash 5c9559092f Remove duplicate group name entry field on group edit form
squash b5b38dc84a Address eslint errors in group.js
squash 91f066f982 Enable group name copy with dispatch and explore group creation
squash a1140a4d3b Update workflow new group instruction
squash f6b008a8e7 Update the gitlab basics group creation document
squash 257b4c474c Add a change log entry
squash 26e38cd290 Remove unused variable for eslint

# Rebase dc99f343af..26e38cd290 onto dc99f343af (18 commands)

    Now, I'm very sure that isn't a good plan. There's that note about removed commits being deleted. In any case, there's no happiness there for me. After saving, I get:

    error: could not apply a550adfd2f... Add a name field to the group edit form

When you have resolved this problem, run "git rebase --continue".
If you prefer to skip this patch, run "git rebase --skip" instead.
To check out the original branch and stop rebasing, run "git rebase --abort".

Could not apply a550adfd2fd567db23e82e00b475c92f87b073bc... Add a name field to the group edit form

    At which point I, git rebase --abort.

    I have also tried first rebasing on master, which is up to date with the remote, git rebase master and I'll show the output for completeness:

    First, rewinding head to replay your work on top of it...
Applying: Add a name field to the group edit form
Applying: Cause new group form to copy name from path
Applying: Remove duplicate group name entry field on group edit form
Applying: Address eslint errors in group.js
Applying: Enable group name copy with dispatch and explore group creation
Applying: Update workflow new group instruction
Applying: Update the gitlab basics group creation document
Applying: Add a change log entry
Applying: Add a name field to the group edit form
Using index info to reconstruct a base tree...
M	app/assets/javascripts/dispatcher.js
M	app/views/shared/_group_form.html.haml
M	config/webpack.config.js
M	doc/gitlab-basics/create-group.md
M	doc/gitlab-basics/img/create_new_group_info.png
M	doc/workflow/groups.md
M	doc/workflow/groups/new_group_form.png
M	spec/features/admin/admin_groups_spec.rb
M	spec/features/dashboard/group_spec.rb
M	spec/features/groups_spec.rb
Falling back to patching base and 3-way merge...
No changes -- Patch already applied.
Applying: Remove unused variable for eslint

    I'm not sure I like git status now:

    On branch add-field-for-group-name
Your branch and 'origin/add-field-for-group-name' have diverged,
and have 373 and 11 different commits each, respectively.
  (use "git pull" to merge the remote branch into yours)
nothing to commit, working tree clean

    But, okay git push? Here's a problem:

    To gitlab.com:dclovell/gitlab-ce.git
 ! [rejected]              add-field-for-group-name -> add-field-for-group-name (non-fast-forward)
error: failed to push some refs to 'git@gitlab.com:dclovell/gitlab-ce.git'
hint: Updates were rejected because the tip of your current branch is behind
hint: its remote counterpart. Integrate the remote changes (e.g.
hint: 'git pull ...') before pushing again.
hint: See the 'Note about fast-forwards' in 'git push --help' for details.

    I've read about fast-forwards in git push --help and don't see anything I'd like to try. git pull gives me a merge, which I save, and then git push succeeds. Now git log --oneline:

    ab164e4e0b Merge branch 'add-field-for-group-name' of gitlab.com:dclovell/gitlab-ce into add-field-for-group-name
87c673ce52 Remove unused variable for eslint
af6eb3e3aa Add a change log entry
5c89682953 Update the gitlab basics group creation document
483868f11f Update workflow new group instruction
da4a8e9920 Enable group name copy with dispatch and explore group creation
c5dca821f9 Address eslint errors in group.js
77c0b3ecec Remove duplicate group name entry field on group edit form
2b62e417b2 Cause new group form to copy name from path
aab5d39728 Add a name field to the group edit form
47a4b6e82f Merge branch 'cycle-analytics-commit-icon' into 'master'
cb6ae9d1a5 Merge branch 'realtime-methods-docs' into 'master'

    I have ten commits including the merge commit or I can say, git rebase -i 47a4b6e82f yields the following to edit:

    pick aab5d39728 Add a name field to the group edit form
pick 2b62e417b2 Cause new group form to copy name from path
pick 77c0b3ecec Remove duplicate group name entry field on group edit form
pick c5dca821f9 Address eslint errors in group.js
pick da4a8e9920 Enable group name copy with dispatch and explore group creation
pick 483868f11f Update workflow new group instruction
pick 5c89682953 Update the gitlab basics group creation document
pick af6eb3e3aa Add a change log entry
pick 87c673ce52 Remove unused variable for eslint
pick c0149cc046 Add a name field to the group edit form
pick a550adfd2f Add a name field to the group edit form
pick bc684b69fa Cause new group form to copy name from path
pick 5c9559092f Remove duplicate group name entry field on group edit form
pick b5b38dc84a Address eslint errors in group.js
pick 91f066f982 Enable group name copy with dispatch and explore group creation
pick a1140a4d3b Update workflow new group instruction
pick f6b008a8e7 Update the gitlab basics group creation document
pick 257b4c474c Add a change log entry
pick 26e38cd290 Remove unused variable for eslint

# Rebase 47a4b6e82f..ab164e4e0b onto 47a4b6e82f (19 commands)

    That has strangely duplicated commit messages, but edit as follows:

    pick aab5d39728 Add a name field to the group edit form
squash 2b62e417b2 Cause new group form to copy name from path
squash 77c0b3ecec Remove duplicate group name entry field on group edit form
squash c5dca821f9 Address eslint errors in group.js
squash da4a8e9920 Enable group name copy with dispatch and explore group creation
squash 483868f11f Update workflow new group instruction
squash 5c89682953 Update the gitlab basics group creation document
squash af6eb3e3aa Add a change log entry
squash 87c673ce52 Remove unused variable for eslint
squash c0149cc046 Add a name field to the group edit form
squash a550adfd2f Add a name field to the group edit form
squash bc684b69fa Cause new group form to copy name from path
squash 5c9559092f Remove duplicate group name entry field on group edit form
squash b5b38dc84a Address eslint errors in group.js
squash 91f066f982 Enable group name copy with dispatch and explore group creation
squash a1140a4d3b Update workflow new group instruction
squash f6b008a8e7 Update the gitlab basics group creation document
squash 257b4c474c Add a change log entry
squash 26e38cd290 Remove unused variable for eslint

# Rebase 47a4b6e82f..ab164e4e0b onto 47a4b6e82f (19 commands)

    On save, I get:

    error: could not apply c0149cc046... Add a name field to the group edit form

When you have resolved this problem, run "git rebase --continue".
If you prefer to skip this patch, run "git rebase --skip" instead.
To check out the original branch and stop rebasing, run "git rebase --abort".

Could not apply c0149cc0460440c3b6994b6d370035bba024eede... Add a name field to the group edit form

    and then git rebase --abort

    Again for completeness. Here's the content of my .git/config

    [core]
	repositoryformatversion = 0
	filemode = true
	bare = false
	logallrefupdates = true
	ignorecase = true
	precomposeunicode = true
[remote "origin"]
	url = git@gitlab.com:dclovell/gitlab-ce.git
	fetch = +refs/heads/*:refs/remotes/origin/*
[branch "master"]
	remote = upstream
	merge = refs/heads/master
[remote "upstream"]
	url = https://gitlab.com/gitlab-org/gitlab-ce.git
	fetch = +refs/heads/*:refs/remotes/upstream/*
	pushurl = none
[branch "add-field-for-group-name"]
	remote = origin
	merge = refs/heads/add-field-for-group-name
  • username-removed-1181022 marked the task Branch has no merge conflicts with master (if it does - rebase it please) as completed

    marked the task Branch has no merge conflicts with master (if it does - rebase it please) as completed

  • username-removed-1181022 marked the task All builds are passing as completed

    marked the task All builds are passing as completed

  • username-removed-1181022 unmarked as a Work In Progress

    unmarked as a Work In Progress

  • username-removed-1181022 mentioned in issue #25474 (moved)

    mentioned in issue #25474 (moved)

  • username-removed-408677
    username-removed-408677 @ClemMakesApps ·
    Maintainer

    Oh man @dclovell. I just checked with the other team members and we now have (https://docs.gitlab.com/ee/user/project/merge_requests/squash_and_merge.html#commit-metadata-for-squashed-commits) so as long as your branch is working, don't worry about the commits and squashing for now :thumbsup:

  • username-removed-1181022
    username-removed-1181022 @dclovell ·
    Author

    That is such a relief. Thanks to the people who implemented that. And thank you @ClemMakesApps .

  • username-removed-1181022
    username-removed-1181022 @dclovell ·
    Author

    Now that the monthly release cycle is wrapping up, I have a question:

    What are the processes/expectations/timelines around reviewing and integrating community contribution MRs? How should I level set expectation? My current expectation is sometime within the next release cycle reviewed, comments addressed, merged.

  • username-removed-408677
    username-removed-408677 @ClemMakesApps ·
    Maintainer

    Sorry, this slipped through the cracks. We try to have a decent turn around time for Community Contribution MRs. @iamphill can you review this?

  • Phil Hughes
  • Phil Hughes
  • Phil Hughes
  • Phil Hughes
    Phil Hughes @iamphill started a thread on an old version of the diff
    • app/assets/javascripts/dispatcher.js
    134 134 case 'dashboard:groups:index':
    135 135 case 'explore:groups:index':
    136 136 new GroupsList();
    137 new gl.Group();
  • Phil Hughes
    Phil Hughes @iamphill ·
    Maintainer

    @dclovell Few comments. & noticed a bug, if I edit the path it updates the name. If I then update the name but decide to change the path it then overwrites the name I just changed.

  • Phil Hughes assigned to @dclovell

    assigned to @dclovell

  • username-removed-1181022
    username-removed-1181022 @dclovell ·
    Author

    Thank you very much for the comments. I have made one commit to address the most prominent issues. Will address the remainder shortly and follow-through.

  • username-removed-1181022 added 1 commit

    added 1 commit

    • 95c3e214 - Test and fix edit overwrite bug

    Compare with previous version

  • username-removed-1181022 added 2 commits

    added 2 commits

    • e5007830 - Use export/import over window.gl for Group class
    • 13b9c8fa - Remove unneeded Group js instance on groups:index

    Compare with previous version

  • username-removed-1181022 added 786 commits

    added 786 commits

    Compare with previous version

  • username-removed-1181022 added 31 commits

    added 31 commits

    Compare with previous version

  • username-removed-1181022
    username-removed-1181022 @dclovell ·
    Author

    @iamphill Hi Phil: I've addressed all the issues, and doubled the time needed to do that by trying to shepherd this through CI again. Currently all of the spinach test suites are failing with, "rack-test requires a rack application, but none was given". The error as nothing to do with these changes, I think.

    Please have another look. I'm not marking the discussions resolved. I figured that's up to you whether they are resolved to your satisfaction.

  • Phil Hughes assigned to @alfredo1

    assigned to @alfredo1

  • Phil Hughes
    Phil Hughes @iamphill ·
    Maintainer

    @dclovell I think that error is known, so you can rebase against to get the latest fixes :thumbsup: https://gitlab.com/gitlab-org/gitlab-ce/issues/30347

  • username-removed-1181022 added 171 commits

    added 171 commits

    Compare with previous version

  • username-removed-1181022
    username-removed-1181022 @dclovell ·
    Author

    No. The spinach tests are still failing. So does it really take three weeks to get a feature implementation through your process? This was something relatively simple. Well, we're still not done. @alfredo1 @iamphill

    The error is ArgumentError: rack-test requires a rack application, but none was given and comes from spinach. Some sort of build/test environment issue? Caused by this code?

    The rest of the tests will pass. Some will time-out if your CI servers are running slowly and require retry.

  • username-removed-408881
    username-removed-408881 @asumaran ·
    Contributor

    @dclovell that is a known issue. I'll review and merge if test errors are unrelated.

  • username-removed-408881
    username-removed-408881 @asumaran ·
    Contributor

    Code LGTM, @smcgivern can you check the ruby code please?

  • username-removed-408881 assigned to @smcgivern

    assigned to @smcgivern

  • username-removed-443319
    username-removed-443319 @smcgivern ·
    Contributor

    @alfredo1 this LGTM. Thanks @dclovell for your hard work and patience on this, and for updating the docs! :books:

  • username-removed-443319 assigned to @alfredo1

    assigned to @alfredo1

  • username-removed-408881
    username-removed-408881 @asumaran ·
    Contributor

    @dclovell There are some conflicts. Can you fix them? Thanks!

  • username-removed-408881 mentioned in merge request !10502 (merged)

    mentioned in merge request !10502 (merged)

  • username-removed-408881
    username-removed-408881 @asumaran ·
    Contributor

    @dclovell I've created another MR where I fixed conflicts. I'll merge when test passes https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/10502 Thanks!

  • username-removed-408881 closed

    closed

  • username-removed-1181022
    username-removed-1181022 @dclovell ·
    Author

    Woohoo! Thank you all. Happy to contribute.

    • Please register or sign in to reply