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

Improve archiving function #2008

Merged
merged 4 commits into from
Jan 27, 2016
Merged

Improve archiving function #2008

merged 4 commits into from
Jan 27, 2016

Conversation

matthias-brun
Copy link
Contributor

This pr makes it so users that don't have the appropriate permissions to archive a channel aren't shown the Archiving button but instead simply see the current state of the room. ( i.e. 'active' or 'archived')
Those users who do have the necessary permissions also get the option to edit the state, which now works the same way that editing the room type ('private', 'channel') does.
screen shot 2016-01-22 at 18 00 09
screen shot 2016-01-22 at 18 00 15

@marceloschmidt
Copy link
Member

lgtm

@@ -106,4 +98,13 @@ Template.channelSettings.onCreated ->
return toastr.error TAPi18n.__(err.reason, err.details.roomType)
return toastr.error TAPi18n.__(err.reason)
toastr.success TAPi18n.__ 'Room_type_changed_successfully'
when 'archivationState'
if @$('input[name=archivationState]:checked').val() is 'true'
Meteor.call 'archiveRoom', @data?.rid, (err, results) ->
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't check to not call archiveRoom in already archived rooms? The same for unarchiveRoom.

Copy link
Contributor

Choose a reason for hiding this comment

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

What exactly do you mean by that @rodrigok? The first sentence contains a double negative, so I can't exactly make out what you're really saying...I assume it means something, but not 100% sure and we all know what they say about assuming things. 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@graywolf336 What he means is that before calling archiveRoom or unarchiveRoom there should be a check whether it is even necessary to archive the room or if it is archived already. (i.e. If the user edits the settings of an archived channel, leaves the radio button on archived and clicks on save.)
@rodrigok I'll make sure to add that in.

Copy link
Member

Choose a reason for hiding this comment

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

@graywolf336 sorry about that, I will try to explain better on the next time, fell free to correct me, I'm learning english yet 😄

@matthias-brun thanks 😃

@matthias-brun
Copy link
Contributor Author

Okay, I added in the check we discussed.

I was also thinking it might make sense to integrate archiving in the saveRoomSettings method (which is used for the room name/topic/type) to ensure that behaviour is consistent (and the code is consistently structured).
But I'll leave that decision to you guys since I'm not sure about what implications that change might have.

@rodrigok
Copy link
Member

@matthias-brun IMHO separated methods are a better solution, more clear and we can wrap for APIs like REST API.

@RocketChat/core what do you think?

@rodrigok
Copy link
Member

LGTM

@graywolf336
Copy link
Contributor

@rodrigok Yes, I agree that having different methods is better solution as I had trouble figuring out how to save a room's name after the update for the importers.

lgtm

engelgabriel added a commit that referenced this pull request Jan 27, 2016
@engelgabriel engelgabriel merged commit 22ed7d6 into RocketChat:develop Jan 27, 2016
@matthias-brun matthias-brun deleted the improve-archiving-function branch January 29, 2016 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants