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

Regression: "Enable unlimited apps" button on installation modal doesn't do anything #28132

Conversation

AllanPazRibeiro
Copy link
Contributor

@AllanPazRibeiro AllanPazRibeiro commented Feb 23, 2023

Proposed changes (including videos or screenshots)

This PR fixes all buttons on the "Enable unlimited apps" modal, and the problems were:

  • Close button: This button was just a 'placebo', now it's calling the setModal(null) properly inside a useCallback, which I noticed is a good practice and used on most of our modals.
  • Contact sellers: This one was concatenating with the server's current path, so it was pointing to a non-existent URL like: http://localhost:3000/app/admin/go.rocket.chat /contact-sellers. In order to fix it I change it to an HTTPS URL having the target _blank.
  • Start trial: Last but not least, this one was having a very similar problem to the previous one, but the redirect was faster than some requests so the application was not been able to check the permissions to show the page and at the same time the modal was not been closed. In order to fix it, I replace the Flowrouter to a more native approach (window), so now the page is refreshing and getting all the permissions needed.

Issue(s)

MKP-246

Steps to test or reproduce

Further comments

MKP-246
MKP-255
MKP-256
MKP-257

@codecov
Copy link

codecov bot commented Feb 23, 2023

Codecov Report

Merging #28132 (06cb96b) into develop (fe1a36e) will decrease coverage by 1.08%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop   #28132      +/-   ##
===========================================
- Coverage    44.94%   43.87%   -1.08%     
===========================================
  Files          764      754      -10     
  Lines        14897    14682     -215     
  Branches      2069     2054      -15     
===========================================
- Hits          6695     6441     -254     
- Misses        7911     7973      +62     
+ Partials       291      268      -23     
Flag Coverage Δ
e2e 43.82% <ø> (-1.08%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

@debdutdeb debdutdeb changed the title [FIX]: Enable unlimited apps button on installation modal doesn't do anything [FIX] Enable unlimited apps button on installation modal doesn't do anything Feb 23, 2023
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.

Approved by mistake 😬 Previous comments must be solved

@d-gubert d-gubert changed the title [FIX] Enable unlimited apps button on installation modal doesn't do anything Regression: "Enable unlimited apps" button on installation modal doesn't do anything Feb 24, 2023
@d-gubert d-gubert added this to the 6.0.0 milestone Feb 27, 2023
@dionisio-bot dionisio-bot bot added the stat: ready to merge PR tested and approved waiting for merge label Feb 27, 2023
rique223
rique223 previously approved these changes Feb 27, 2023
@kodiakhq kodiakhq bot removed the stat: ready to merge PR tested and approved waiting for merge label Feb 28, 2023
@kodiakhq
Copy link
Contributor

kodiakhq bot commented Feb 28, 2023

This PR currently has a merge conflict. Please resolve this and then re-add the ['stat: ready to merge', 'automerge'] label.

…imited-apps-button-on-installation-modal-doesnt-do-anything
@d-gubert d-gubert added the stat: ready to merge PR tested and approved waiting for merge label Feb 28, 2023
@dionisio-bot dionisio-bot bot removed the stat: ready to merge PR tested and approved waiting for merge label Feb 28, 2023
@dionisio-bot dionisio-bot bot added the stat: ready to merge PR tested and approved waiting for merge label Feb 28, 2023
@dionisio-bot dionisio-bot bot added stat: ready to merge PR tested and approved waiting for merge and removed stat: ready to merge PR tested and approved waiting for merge labels Feb 28, 2023
@kodiakhq kodiakhq bot merged commit 9aaec9f into develop Feb 28, 2023
@kodiakhq kodiakhq bot deleted the MKP-246-enable-unlimited-apps-button-on-installation-modal-doesnt-do-anything branch February 28, 2023 20:53
@sampaiodiego sampaiodiego mentioned this pull request Mar 9, 2023
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stat: QA tested stat: ready to merge PR tested and approved waiting for merge type: regression
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants