Add a banner to encrypted rooms with visible history. #4851
Conversation
kaylendog
added
the
T-Enhancement
pixlwave
added
pr-wip
|
Any chance you could squash everything from #4738 into a single commit so that it's easy to see what was reviewed already and the changes that this PR will make on top of that? 😇 |
* feat: Add history visible alert. - Adds a dismissable alert that is displayed whenever the user opens a room with `history_visibility` != `joined`. When cleared, this is recorded in the app's data store. - When opening a room with `history_visibility` = `joined`, this flag is cleared. Issue: element-hq/element-meta#2875 * tests: Add unit tests for history sharing in `RoomScreenFooterView`. * feat: Rename flag to `hasSeenHistoryVisibleBannerRooms`, document. * refactor: Merge enum variants, use function for banner description. * feat: Use `AppSettings.historyVisibleDetailsURL` over hard-coded value. * tests: Correct potential race condition with deferred assertion. * chore: Use Localazy translation string over project-defined. * fix: Final tweaks and review comments. * chore: Checkout `Enterprise` submodule. * tests: Final fixes.
kaylendog
force-pushed
the
kaylendog/history-sharing/alert
branch
from
9c05308 to
4020623
Compare
| ServiceLocator.shared.settings.enableKeyShareOnInvite = true | ||
| ServiceLocator.shared.settings.acknowledgedHistoryVisibleRooms = Set() |
There was a problem hiding this comment.
I'm not convinced resetting the set every test is necessary, but after I set prod on fire I don't want to take any chances.
There was a problem hiding this comment.
after I set prod on fire
Just for the record, it only made it to nightly before we reverted it, and while we generally aim not to break nightly either this was more of a minor annoyance.
There was a problem hiding this comment.
💯 – this is the entire point of Nightly builds and instability should be expected with them from time to time.
|
|
I've had a look through the CI logs, but can't actually see any unit test that's failing - either I'm blind or that's a false negative? |
|
It was just a flakey test, searching for Given there's a small conflict with #4861 (it also includes the element.io URL) and that it improves the copy, I'd like to wait for that to merge and for this to be rebased before I review these changes again if you don't mind? |
3 participants
Take no. 2 - conditioned the banner appearance on the feature flag, added another unit test that verifies the incorrect behaviour reported no longer happens.
Pull Request Checklist
UI changes have been tested with: