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: Remove 4.0 version banner #26251

Merged
merged 4 commits into from
Jul 19, 2022
Merged

Conversation

hugocostadev
Copy link
Contributor

Proposed changes (including videos or screenshots)

Created a migration to disable and dismiss for all users the old 4.0 version banner.
It happened when a new admin user has been added.

Issue(s)

Steps to test or reproduce

Further comments

filipemarins
filipemarins previously approved these changes Jul 14, 2022
@filipemarins filipemarins added this to the 5.0.0 milestone Jul 15, 2022
Copy link
Member

@sampaiodiego sampaiodiego left a comment

Choose a reason for hiding this comment

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

I'm sorry, but where is this banner coming from?

I'd like to understand as this doesn't look like to be the best implementation for this purpose.

@hugocostadev
Copy link
Contributor Author

I'm sorry, but where is this banner coming from?

I'd like to understand as this doesn't look like to be the best implementation for this purpose.

Hi @sampaiodiego, this banner was added in a previous migration (v231) and the migration v232 tried to dismiss, but the problem is that the v232 migration just dismisses for admins if a user is added as admin after this migration execution the user is able to see the banner. This issue can be verified at: https://unstable.rocket.chat/

@sampaiodiego
Copy link
Member

sampaiodiego commented Jul 18, 2022

thx @hugocostadev for the explanation.. so if I get it right, now the ideia is to dismiss the banner to everyone, right? in this case, we don't need to actually dismiss, we can just disable the banner itself.. once it is disabled, no one will see it.

and to disable it, since we're doing it in a migration, we can do it calling the model directly.. the query can be something like:

Banners.updateOne({
  'view.blocks.0.text.text': /authentication\-changes/,
  active: { $ne: false }
}, {
  $set: {
    active: false,
    inactivedAt: new Date(),
  },
})

that should be enough :)

btw, we can use this banner as an example why we should chose for specific IDs when we create a banner ourselves, so it is much easier to deal with it if needed.

I'm going to add other comments to the code, to explain what we should avoid in the proposed solution

Copy link
Member

@sampaiodiego sampaiodiego left a comment

Choose a reason for hiding this comment

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

I've explained what we should avoid and why..

so please, you can change everything starting from line 26, and perform only the Banner from my previous comment:

Banners.updateOne({
  'view.blocks.0.text.text': /authentication\-changes/,
  active: { $ne: false }
}, {
  $set: {
    active: false,
    inactivedAt: new Date(),
  },
})

@hugocostadev
Copy link
Contributor Author

I've explained what we should avoid and why..

so please, you can change everything starting from line 26, and perform only the Banner from my previous comment:

Banners.updateOne({
  'view.blocks.0.text.text': /authentication\-changes/,
  active: { $ne: false }
}, {
  $set: {
    active: false,
    inactivedAt: new Date(),
  },
})

Perfect Diego, I got it! Thanks so much for your explanations, indeed this is a much cleaner and wise solution

@ggazzo ggazzo added stat: ready to merge PR tested and approved waiting for merge and removed stat: needs QA labels Jul 19, 2022
@ggazzo ggazzo merged commit ca50f0f into develop Jul 19, 2022
@ggazzo ggazzo deleted the fix/remove-version-banner branch July 19, 2022 17:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants