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.

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

Allow renaming of requests #12

Merged
merged 1 commit into from
Sep 21, 2017
Merged

Allow renaming of requests #12

merged 1 commit into from
Sep 21, 2017

Conversation

mrsimpson
Copy link
Member

@mrsimpson mrsimpson commented Sep 20, 2017

Fixes #11

@mrsimpson mrsimpson self-assigned this Sep 20, 2017
@mrsimpson mrsimpson requested a review from ruKurz September 20, 2017 13:57
@ruKurz
Copy link

ruKurz commented Sep 20, 2017

@mrsimpson I commented your changes. But I'm not able to validate this PR in detail.
How to proceed. Should I accept the PR, by a local test?

From my point of view the code changes do not cause any error so I would suggest, merging the request, right now would be the best.

@mrsimpson
Copy link
Member Author

@ruKurz to perform a local test. Create a request, open the channel setting tab bar (i)-Icon and rename it.
But it's really not huge, if you don't have any concerns, merge it

@@ -58,10 +58,10 @@ Template.AssistifyCreateRequest.events({
console.log(err);
switch (err.error) {
case 'error-invalid-name':
toastr.error(TAPi18n.__('Invalid_room_name', name));
toastr.error(TAPi18n.__('Invalid_room_name', `${ expertise }...`));
Copy link

Choose a reason for hiding this comment

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

seems like this is a real mistake, since 'expertise' has been there before.
Could it be that 'name' is a relict from a previous refactoring or this is relict by copy & paste?

return;
case 'error-duplicate-channel-name':
toastr.error(TAPi18n.__('Duplicate_channel_name', name));
toastr.error(TAPi18n.__('Request_already_exists'));
Copy link

Choose a reason for hiding this comment

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

follow up from previous fix

@@ -170,7 +170,7 @@ Template.channelSettings.onCreated(function() {
},
save(value, room) {
let nameValidation;
if (!RocketChat.authz.hasAllPermission('edit-room', room._id) || (room.t !== 'c' && room.t !== 'p')) {
if (!RocketChat.authz.hasAllPermission('edit-room', room._id) || (room.t !== 'c' && room.t !== 'p' && room.t !== 'e' && room.t !== 'r')) {
Copy link

Choose a reason for hiding this comment

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

what is the semantic of 'room.t' values.
Since I'm not that familiar with RC 'c', 'p', 'e', 'r' is not self explaining, even one could suggest.
Also I do not have any clue how the room type and the name vs. expertise thing belong together.

@ruKurz ruKurz merged commit 531d2af into master Sep 21, 2017
@ruKurz ruKurz deleted the hotfix/#8-rename-requests branch September 21, 2017 12:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants