Skip to content
This repository was archived by the owner on Apr 17, 2023. It is now read-only.

Added the possibility to rename the team.#536

Merged
mssola merged 1 commit intoSUSE:masterfrom
jloehel:renaming_team_2
Nov 11, 2015
Merged

Added the possibility to rename the team.#536
mssola merged 1 commit intoSUSE:masterfrom
jloehel:renaming_team_2

Conversation

@jloehel
Copy link
Copy Markdown
Contributor

@jloehel jloehel commented Nov 5, 2015

Screenshot:
screenshot - 11052015 - 03 00 37 pm

Signed-off-by: Jürgen Löhel jloehel@suse.com

@mssola
Copy link
Copy Markdown
Collaborator

mssola commented Nov 6, 2015

Cool 👏 Some comments:

Take a look at the generated activity:

lala2

This is not that useful, isn't it? It would be better to say something like: "mssola renamed the team asd to new-name". Basically:

  • It's weird to say that you've "edited a name": you "rename" ("changed the name" could work too).
  • You should provide what was the previous name, and what's the current name. The link should be on the new name. Note that this can be done through activity parameters.

Second of all, the ChangeName and the ChangeDescription concerns can be merged. They're almost identical. What I would do is:

  1. Create a concern named ChangeNameDescription.
  2. This concern would have a public method: change_name_description(object, symbol).
  3. Depending on the changed column, it would create one kind of activity or another. Note that you can update both the name and the description when performing the update (if the user hasn't changed the name, well, it will update to the same thing :P). With this in mind the implementation should become simpler.

On the implementation of this method, I wouldn't be shy with helper protected methods. That is, take advantage of the similarities between both concerns. Note that you can make this work even for models that do not have an editable name (e.g. Namespace). To be more certain, I'd write a new test like this:

  1. Check that it works for a mock model with just an editable description.
  2. Check that it works for a mock model with just an editable description.

You don't have to be too exhaustive on this test, but it would be a nice addition.

On the form itself, I'd make the name input "required". This is because a team always has to have a name. Note that this would also work even if the users is just modifying the description.

@jloehel jloehel changed the title Added the possibility to rename the team name. Added the possibility to rename the team. Nov 6, 2015
Comment thread app/views/teams/show.html.slim Outdated
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Add tabindex to 1.

@mssola
Copy link
Copy Markdown
Collaborator

mssola commented Nov 10, 2015

Just fix these small issues and it will be good to go :)

@jloehel jloehel force-pushed the renaming_team_2 branch 2 times, most recently from a96e434 to 38ecf22 Compare November 11, 2015 09:10
@mssola
Copy link
Copy Markdown
Collaborator

mssola commented Nov 11, 2015

Could you rebase your commit? Right now it has conflicts.

Signed-off-by: Jürgen Löhel <jloehel@suse.com>
mssola added a commit that referenced this pull request Nov 11, 2015
Added the possibility to rename the team.
@mssola mssola merged commit 5f8b62c into SUSE:master Nov 11, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants