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

Update MSC3912 implementation: Redaction of related events #8532

Merged
merged 2 commits into from Jul 4, 2023

Conversation

yostyle
Copy link
Contributor

@yostyle yostyle commented Jun 13, 2023

Type of change

  • Feature
  • Bugfix
  • Technical
  • Other :

Content

#8481

Motivation and context

Screenshots / GIFs

Tests

  • Step 1
  • Step 2
  • Step ...

Tested devices

  • Physical
  • Emulator
  • OS version(s):

Checklist

@yostyle yostyle requested review from a team, bmarty and giomfo and removed request for a team June 13, 2023 14:47
Copy link
Member

@giomfo giomfo left a comment

Choose a reason for hiding this comment

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

@yostyle commented, let me know in DM if you need more details

@@ -165,6 +164,5 @@ internal fun Versions.doesServerSupportRemoteToggleOfPushNotifications(): Boolea
* @return true if event redaction with relations is supported
*/
internal fun Versions.doesServerSupportRedactEventWithRelations(): Boolean {
Copy link
Member

Choose a reason for hiding this comment

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

This method just let you know if the feature is supported or not. There is no way to know if this feature is unstable or not. This is a potential pb, because the name of the attribute depends on the stability of the feature.

This remark is not limited to this feature. This concerns all the features handled here.
I don't know if @bmarty wants to improve this here. This remark has to be taken into account at least for EX.

Here I would add a second method
doesServerSupportStableRedactionOfRelatedEvents
and rename the current one: doesServerSupportUnstableRedactionOfRelatedEvents

val withRelations: List<String>? = null,
)
) {
fun getBestWithRelations() = withRelations ?: unstableWithRelations
Copy link
Member

Choose a reason for hiding this comment

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

This function should select the right option according to the stability of the feature

@sonarcloud
Copy link

sonarcloud bot commented Jun 15, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

Copy link
Member

@giomfo giomfo left a comment

Choose a reason for hiding this comment

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

SGTM

Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

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

LGTM

@yostyle yostyle merged commit 0573915 into develop Jul 4, 2023
16 of 17 checks passed
@yostyle yostyle deleted the yostyle/MSC3912_update branch July 4, 2023 13:12
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.

None yet

3 participants