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

[NEW] Add possibility to set room closer to LivechatUpdater.closeRoom #391

Conversation

cuonghuunguyen
Copy link
Contributor

@cuonghuunguyen cuonghuunguyen commented Mar 7, 2021

What? β›΅

Why? πŸ€”

When closing the room with the Apps, the room closer and the close room message sender will always be set to the visitor. This is not a correct behavior because the visitor does not perform that function.
This is not a very clean approach, we actually should pass something like closeData which contains all the needed data for Rocket.Chat close room function, but to avoid breaking change we can accept this approach right now

Links 🌎

Rocket.Chat issue: RocketChat/Rocket.Chat#21025

PS πŸ‘€

@CLAassistant
Copy link

CLAassistant commented Mar 7, 2021

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Mar 7, 2021

Codecov Report

Merging #391 (7cd690f) into alpha (f1c071d) will increase coverage by 1.23%.
The diff coverage is 100.00%.

❗ Current head 7cd690f differs from pull request most recent head fe47531. Consider uploading reports for the commit fe47531 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##            alpha     #391      +/-   ##
==========================================
+ Coverage   48.67%   49.91%   +1.23%     
==========================================
  Files         103      110       +7     
  Lines        3244     3370     +126     
  Branches      476      473       -3     
==========================================
+ Hits         1579     1682     +103     
- Misses       1665     1688      +23     
Impacted Files Coverage Ξ”
src/server/accessors/LivechatUpdater.ts 44.44% <100.00%> (+22.22%) ⬆️
src/server/compiler/modules/net.ts 50.00% <0.00%> (-50.00%) ⬇️
...c/server/permissions/checkers/AppInternalBridge.ts 20.00% <0.00%> (-30.00%) ⬇️
...issions/checkers/AppEnvironmentalVariableBridge.ts 30.76% <0.00%> (-29.24%) ⬇️
src/server/permissions/checkers/AppApisBridge.ts 36.36% <0.00%> (-26.14%) ⬇️
...ver/permissions/checkers/AppUiInteractionBridge.ts 44.44% <0.00%> (-22.23%) ⬇️
...server/permissions/checkers/AppActivationBridge.ts 11.11% <0.00%> (-22.23%) ⬇️
src/server/permissions/checkers/AppUploadBridge.ts 25.00% <0.00%> (-21.16%) ⬇️
.../server/permissions/checkers/AppSchedulerBridge.ts 25.00% <0.00%> (-21.16%) ⬇️
src/server/errors/PermissionDeniedError.ts 14.28% <0.00%> (-19.05%) ⬇️
... and 22 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Ξ” = absolute <relative> (impact), ΓΈ = not affected, ? = missing data
Powered by Codecov. Last update b574248...fe47531. Read the comment docs.

@cuonghuunguyen cuonghuunguyen force-pushed the meomay503/possibility-to-set-room-373 branch 4 times, most recently from f6b943f to 7cd690f Compare March 7, 2021 10:29
@lucassartor
Copy link
Contributor

Hi @meomay503! Can you share the app you've made to test the functionality? Thanks πŸ€—

@lucassartor lucassartor self-requested a review March 24, 2021 17:58
@cuonghuunguyen
Copy link
Contributor Author

Hi, thanks for having a look at this PR.
clean-all-livechat-room_0.0.1.zip
I've created a simple app to test this. It basically clean all opening livechat rooms by the closer is rocket.cat by the slash command /clean-livechat
Pls remember to link the correct version for Apps-Engine and also Rocket.Chat.

@cuonghuunguyen
Copy link
Contributor Author

Do u have any updates on this?

@debdutdeb debdutdeb requested review from d-gubert and removed request for lucassartor September 4, 2021 14:24
@graywolf336
Copy link
Contributor

@cuonghuunguyen any chance you can update this PR to resolve the conflicts? Once you do and everything is ready to go, I'll internally push this PR forward and try to get it (and all the connected pieces) merged ASAP. My apologies that his has not received any attention. I work on the marketplace now and was going through your Clean Livechat app to review it and seen your reference to this PR. As a result of that, that's why I am more active in this repo :)

@cuonghuunguyen
Copy link
Contributor Author

ok, I'll update this one ASAP

@cuonghuunguyen
Copy link
Contributor Author

hi @graywolf336, I updated the PRs

@cuonghuunguyen cuonghuunguyen force-pushed the meomay503/possibility-to-set-room-373 branch from 7cd690f to c1493f4 Compare December 2, 2021 08:40
@cuonghuunguyen cuonghuunguyen force-pushed the meomay503/possibility-to-set-room-373 branch from c1493f4 to fe47531 Compare December 2, 2021 08:42
@graywolf336
Copy link
Contributor

Just to double check, this is backwards compatible, right? So if I install an App that uses an old apps engine, will it still work even if the rocket.chat server has this version?

@cuonghuunguyen
Copy link
Contributor Author

It still work, the new closer field is optional

Copy link
Member

@d-gubert d-gubert left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution!

@d-gubert d-gubert changed the title Add possibility to set room closer to LivechatUpdater.closeRoom [NEW] Add possibility to set room closer to LivechatUpdater.closeRoom Dec 10, 2021
@d-gubert d-gubert merged commit 9a25a73 into RocketChat:alpha Dec 10, 2021
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.

Possibility to set room closer when closing LiveChat Rooms
5 participants