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

fix: Using the banner _id field as a fallback for the view.viewId field #32552

Merged
merged 4 commits into from
Jun 10, 2024

Conversation

Gustrb
Copy link
Contributor

@Gustrb Gustrb commented Jun 4, 2024

Proposed changes (including videos or screenshots)

We were having issues with the seats taken banner, which would've been un-dismissable because it was expecting it to have a view.viewId field.
But since the banner was created before we had this, it was always empty.
This banner was removed in #31047
And it got into the 6.5.0 release onwards, so whenever someone hadn't dismissed the seatsTaken banner and upgraded their workspace, it would now be un-dismissable and would show a toast with the error 'invalid banner'

Issue(s)

Steps to test or reproduce

  1. Create a workspace at 6.4.9
  2. Go into the License.getMaxActiveUsers and change it to return some fixed amount (this will be easier to test)
  3. Add the number of users that would correspond to 80% of that ammount
  4. Restart the workspace (now there should be a seats taken banner)
  5. Change the workspace version to 6.5.0 (or newer)
  6. Close the seats taken banner

(I've had a few issues trying to run this test, my MongoDB would fail to start properly on 6.4.9. To solve this issue, I've deleted the contents of .meteor/local, which did the trick for me :) )

This should actually close the banner and add it to the dismissed banners collection inside MongoDB

Further comments

I could've just added a new migration that would put the value of _id inside the view.viewId field, but since we already have adaptations to have legacy banners I don't think we should add a new migration just for this :P

JIRA Task: https://rocketchat.atlassian.net/browse/SUP-437

We were having issues with the seats taken banner, which would've been undismissable
because it was expecting it to have a view.viewId field
But since the banner was created before we had this, it was always empty
Copy link

changeset-bot bot commented Jun 4, 2024

🦋 Changeset detected

Latest commit: be9efc2

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 32 packages
Name Type
@rocket.chat/meteor Minor
@rocket.chat/core-typings Minor
@rocket.chat/rest-typings Minor
@rocket.chat/apps Patch
@rocket.chat/core-services Patch
@rocket.chat/cron Patch
@rocket.chat/fuselage-ui-kit Major
@rocket.chat/gazzodown Major
@rocket.chat/livechat Patch
@rocket.chat/model-typings Patch
@rocket.chat/ui-contexts Major
@rocket.chat/account-service Patch
@rocket.chat/authorization-service Patch
@rocket.chat/ddp-streamer Patch
@rocket.chat/omnichannel-transcript Patch
@rocket.chat/presence-service Patch
@rocket.chat/queue-worker Patch
@rocket.chat/stream-hub-service Patch
@rocket.chat/api-client Patch
@rocket.chat/license Patch
@rocket.chat/omnichannel-services Patch
@rocket.chat/pdf-worker Patch
@rocket.chat/presence Patch
rocketchat-services Patch
@rocket.chat/ddp-client Patch
@rocket.chat/uikit-playground Patch
@rocket.chat/models Patch
@rocket.chat/ui-avatar Major
@rocket.chat/ui-client Major
@rocket.chat/ui-video-conf Major
@rocket.chat/web-ui-registration Major
@rocket.chat/instance-status Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Contributor

dionisio-bot bot commented Jun 4, 2024

Looks like this PR is ready to merge! 🎉
If you have any trouble, please check the PR guidelines

Copy link

codecov bot commented Jun 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 56.37%. Comparing base (96e66c8) to head (be9efc2).

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop   #32552      +/-   ##
===========================================
- Coverage    56.41%   56.37%   -0.05%     
===========================================
  Files         2437     2437              
  Lines        53773    53815      +42     
  Branches     11082    11097      +15     
===========================================
+ Hits         30335    30336       +1     
- Misses       20792    20836      +44     
+ Partials      2646     2643       -3     
Flag Coverage Δ
e2e 56.09% <ø> (-0.06%) ⬇️
unit 71.87% <100.00%> (-0.28%) ⬇️

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

@Gustrb Gustrb marked this pull request as ready for review June 4, 2024 21:45
@Gustrb Gustrb requested a review from a team as a code owner June 4, 2024 21:45
@Gustrb Gustrb added this to the 6.10 milestone Jun 4, 2024
@Gustrb Gustrb requested a review from KevLehman June 7, 2024 16:57
@Gustrb Gustrb added the stat: QA assured Means it has been tested and approved by a company insider label Jun 10, 2024
@dionisio-bot dionisio-bot bot added the stat: ready to merge PR tested and approved waiting for merge label Jun 10, 2024
@dionisio-bot dionisio-bot bot removed the stat: ready to merge PR tested and approved waiting for merge label Jun 10, 2024
@Gustrb Gustrb added stat: QA assured Means it has been tested and approved by a company insider and removed stat: QA assured Means it has been tested and approved by a company insider labels Jun 10, 2024
@dionisio-bot dionisio-bot bot added the stat: ready to merge PR tested and approved waiting for merge label Jun 10, 2024
@kodiakhq kodiakhq bot merged commit 1e8bf3f into develop Jun 10, 2024
51 checks passed
@kodiakhq kodiakhq bot deleted the fix/failing-to-dismiss-banner branch June 10, 2024 21:38
This was referenced Jun 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stat: QA assured Means it has been tested and approved by a company insider stat: ready to merge PR tested and approved waiting for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants