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: "Upgrade" button on Calmity Banner should be enabled for all users #3520

Merged

Conversation

iamsamgibbs
Copy link
Contributor

@iamsamgibbs iamsamgibbs commented Oct 29, 2024

Description

This PR ensures the "Upgrade" button on the Calamity Banner is always enabled for all users when a new colony version is available.

Testing

To save you the hassle of deploying a colony on an older version, we must first update the canColonyBeUpgraded function in src/utils/checks/canColonyBeUpgraded.ts to return true.

  • Step 1 - Connect to a colony as a user with root permissions. Confirm the Calamity Banner is visible and the upgrade button is enabled.
Screenshot 2024-10-29 at 15 13 47
  • Step 2 - Connect to a colony as a user without root permissions. Confirm the Calamity Banner is visible and the upgrade button is enabled.

Diffs

Deletions ⚰️

  • Removed canUpgradeColony and related uses from useCalamityBannerInfo

Resolves #3340

@iamsamgibbs iamsamgibbs self-assigned this Oct 29, 2024
@iamsamgibbs iamsamgibbs marked this pull request as ready for review October 29, 2024 15:15
@iamsamgibbs iamsamgibbs requested review from a team as code owners October 29, 2024 15:15
@arrenv
Copy link
Member

arrenv commented Oct 29, 2024

@iamsamgibbs Thank you for picking this up, sorry for the confusion on this issue as you have done the work already. @melyndav happened to not have Reputation enabled when running into this originally.

However, hiding the banner completely for everyone but root is not the desired behavior. As it could pose a security risk, and everyone should be informed of a new version.

We already handle permissions in the Upgrade colony version action, and we also allow a colony to be upgraded using Reputation.

So, the correct behavior here should be that the upgrade button is never disabled, and should always open the "Upgrade colony version" action.

Acceptance criteria

  • Upgrade notice calamity banner should appear for everyone.
  • Upgrade button should be enabled for all members.
  • Clicking the upgrade button should open the "Upgrade colony version" action.

Copy link
Member

@arrenv arrenv left a comment

Choose a reason for hiding this comment

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

As above.

@iamsamgibbs iamsamgibbs force-pushed the fix/3340-only-show-calamity-banner-to-users-with-root branch from 18e8a9c to 4d89f1e Compare October 29, 2024 18:17
@iamsamgibbs
Copy link
Contributor Author

No problem Arren! Updated to match the new specs.

@iamsamgibbs iamsamgibbs requested review from arrenv and a team October 29, 2024 18:23
@iamsamgibbs iamsamgibbs changed the title Fix: Only show Calamity Banner to users with root Fix: "Upgrade" button on Calmity Banner should be enabled for all users Oct 29, 2024
Copy link
Contributor

@rumzledz rumzledz left a comment

Choose a reason for hiding this comment

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

Smashed it Sam! 💪

1. Updated the canColonyBeUpgraded function to return true

Screenshot 2024-10-31 at 18 28 46

2. Connected Leela's wallet (has root perms)

Screenshot 2024-10-31 at 18 29 24

3. Connected Amy's wallet (has no root perms)

Screenshot 2024-10-31 at 18 29 42

Copy link
Member

@rdig rdig left a comment

Choose a reason for hiding this comment

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

Nicely fixed! 💯

As leela (root)

Screenshot from 2024-10-31 17-20-15

As fry

Screenshot from 2024-10-31 17-20-39

Copy link
Member

@arrenv arrenv left a comment

Choose a reason for hiding this comment

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

@iamsamgibbs Thank you, and sorry again for the mess around. Looks good to me :)

@iamsamgibbs iamsamgibbs merged commit 598b10a into master Oct 31, 2024
4 of 6 checks passed
@iamsamgibbs iamsamgibbs deleted the fix/3340-only-show-calamity-banner-to-users-with-root branch October 31, 2024 16:03
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.

Colony version upgrades: Only display sitewide banner to those with ROOT permissions
4 participants