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.

Sign up for GitHub

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

Jump to bottom

[WIP]Request access to join private channels. #443

Closed
wants to merge 18 commits into from
Closed

[WIP]Request access to join private channels. #443

wants to merge 18 commits into from

Conversation

vickyokrm
Copy link

@vickyokrm vickyokrm commented Aug 30, 2018 •
edited
Loading

  1. Request access to join the private room
  2. Adapt the new directory search to display Private Groups
  3. Introduce Secrecy to the private channels.

@ghost ghost assigned vickyokrm Aug 30, 2018
@ghost ghost added the progress:working label Aug 30, 2018
@mrsimpson mrsimpson self-requested a review August 30, 2018 13:33
mrsimpson
mrsimpson requested changes Aug 30, 2018
View reviewed changes
Copy link
Member

@mrsimpson mrsimpson left a comment

Choose a reason for hiding this comment

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

From a user's perspective:

  • I expected private rooms next to channels (second tab)
  • The information about secret private channels should only be there in the private groups

I'll keep on reviewing, but it looks really promising

packages/rocketchat-lib/server/functions/createRoom.js Outdated
@@ -42,9 +42,9 @@ RocketChat.createRoom = function(type, name, owner, members, readOnly, extraData
sysMes: readOnly !== true
});

if (type === 'd') {
/* if (type === 'd') {
Copy link
Member

Choose a reason for hiding this comment

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

why has this been commented?

mrsimpson
mrsimpson previously requested changes Aug 30, 2018
View reviewed changes
Copy link
Member

@mrsimpson mrsimpson left a comment •
edited
Loading

Choose a reason for hiding this comment

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

There are some more kind-of-minor items pending, see above and below. But I believe most of them to be quite minor. Good job!

packages/rocketchat-channel-settings/client/startup/messageTypes.js Outdated
message: 'Room_join_request_accepted',
data(message) {
const room = message.room;
Template.room.events({
Copy link
Member

Choose a reason for hiding this comment

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

This adds a (global) event to the room-template - every time a join-request is being rendered. Should/could this not been done statically (as part of the rooms template, ideally)?

packages/rocketchat-ui/client/views/app/createChannel.js Outdated
@@ -164,6 +170,9 @@ Template.createChannel.events({
},
'change [name="type"]'(e, t) {
t.type.set(e.target.checked ? e.target.value : 'd');
if (!e.target.check) {
Copy link
Member

Choose a reason for hiding this comment

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

How does this reflect the comment? I don't see a validation with respect to public channels only. And afterall: should this not be checked?

tests/chimp-config.js Outdated
@@ -19,7 +19,7 @@ module.exports = {
// showXolvioMessages: true,

// // - - - - CUCUMBER - - - -
path: 'tests/end-to-end',
path: 'tests/end-to-end/ui_dummy',
Copy link
Member

Choose a reason for hiding this comment

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

?

@ruKurz
Copy link

ruKurz commented Sep 3, 2018 •
edited by vickyokrm
Loading

  • If a user is removed actively from a private channel he is not able to join it again (Button not displayed)
  • channel content that was written before the request has been approved, is not showing before the requester is refreshing the browser
  • use the lock symbol in the discovery dialog for private channels
  • The wording for visibility should be "versteckt" instead "geheim"
  • Notifications are ending on English verbs on "pending/declined/...", German translation needed
  • Check German translations (e.g. dialog title label to join a private channel should be "Kanal beitreten")

@ruKurz ruKurz self-requested a review September 3, 2018 14:04
ruKurz
ruKurz suggested changes Sep 3, 2018
View reviewed changes
Copy link

@ruKurz ruKurz left a comment

Choose a reason for hiding this comment

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

see the comments above.

We should at least make sure an previously removed user has the option to request access to a channel again.

In general the German translations should be checked.

@ghost ghost assigned mrsimpson Sep 3, 2018
@mrsimpson
Copy link
Member

@vickyokrm with 0.69, there is a new API in RC which renders buttons:
RocketChat#11473

@jschirrmacher jschirrmacher mentioned this pull request Sep 17, 2018
Closed
@ubarsaiyan @vickyokrm
[NEW] Rich message text and image buttons (RocketChat#11473)
afc34c0 
* Added text and image buttons

Added image button with msg

Fixed according to updated JSON

Added horizontal buttons support

Fixed indentation

Added text and image buttons

* Added image button with msg

* Fixed according to updated JSON

* Added horizontal buttons support

* fix style logic

* Fixed lint errors and merge conflict

* Remove lint errors

* Added Actions schema to tests
@janrudolph janrudolph added this to the iteration-27 milestone Oct 1, 2018
@vickyokrm vickyokrm changed the title Request access to join private channels. [WIP]Request access to join private channels. Oct 7, 2018
@vickyokrm vickyokrm changed the base branch from less-is-more to develop October 7, 2018 19:56
mrsimpson
mrsimpson reviewed Oct 11, 2018
View reviewed changes
Copy link
Member

@mrsimpson mrsimpson left a comment

Choose a reason for hiding this comment

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

That looks really good to me! Since it's quite an extensive PR which will result in a lot of merge conflicts in future, I'd like to get some feedback from Rocket.Chat prior to merging it though.

packages/rocketchat-lib/client/lib/openRoom.js
} else {
delete record.$loki; // TODO: Why loki used here
Copy link
Member

Choose a reason for hiding this comment

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

why did you remove it? Did that cause issues?

@mrsimpson mrsimpson dismissed their stale review October 11, 2018 06:58

Changed significantly

@ruKurz
Copy link

ruKurz commented Mar 26, 2019

What is keeping us away merging this PR?

@mrsimpson
Copy link
Member

Unfortunately, we didn't push this to a core PR. We'll not enlarge our diff, so if we should need this again, we'll keep this closed PR as reference

@mrsimpson mrsimpson closed this Dec 7, 2020
@mrsimpson mrsimpson deleted the join-private-channel branch December 7, 2020 09:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mrsimpson mrsimpson mrsimpson left review comments

@ruKurz ruKurz ruKurz requested changes

Assignees

@mrsimpson mrsimpson

@vickyokrm vickyokrm

Labels
progress:working
Projects
None yet
Milestone
iteration-27
Development

Successfully merging this pull request may close these issues.
5 participants
@vickyokrm @ruKurz @mrsimpson @janrudolph @ubarsaiyan