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

Federation Data Exchange for FE2 #1943

Merged
merged 18 commits into from
Jun 27, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -25,15 +25,15 @@ class FederationResult

init {
when (status) {
Copy link
Contributor

@Kaz-ookid Kaz-ookid Jun 27, 2024

Choose a reason for hiding this comment

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

This looks like input validation, maybe you should complete MessageValidator class to check a federation status? Validation would then just be a clear oneliner 👍

You also probably want to check the public key to be non empty B64

"failure" -> {
require(reason != null) { "Reason must be provided for failure status." }
require(publicKey == null) { "Public key must be null for failure status." }
FAILURE -> {
require(reason != null) { "Reason must be provided for $FAILURE status." }
require(publicKey == null) { "Public key must be null for $FAILURE status." }
}
"success" -> {
require(publicKey != null) { "Public key must be provided for success status." }
require(reason == null) { "Reason must be null for success status." }
SUCCESS -> {
require(publicKey != null) { "Public key must be provided for $SUCCESS status." }
require(reason == null) { "Reason must be null for $SUCCESS status." }
}
else -> throw IllegalArgumentException("Status must be either 'failure' or 'success'.")
else -> throw IllegalArgumentException("Status must be either '$FAILURE' or '$SUCCESS'.")
}
}

Expand Down Expand Up @@ -62,16 +62,21 @@ class FederationResult
}

override fun toString(): String {
if (status == "failure") {
if (status == FAILURE) {
return "FederationResult{status='$status', reason='$reason', challenge='$challenge'}"
} else if (status == "success") {
} else if (status == SUCCESS) {
return "FederationResult{status='$status', public_key='$publicKey', " +
"challenge='$challenge'}"
}
return "FederationResult{ERROR}"
}

fun isSuccess(): Boolean {
return status == "success"
return status == SUCCESS
}

companion object {
private const val SUCCESS = "success"
private const val FAILURE = "failure"
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import com.github.dedis.popstellar.repository.LinkedOrganizationsRepository
import com.github.dedis.popstellar.repository.RollCallRepository
import com.github.dedis.popstellar.utility.error.UnknownLaoException
import com.github.dedis.popstellar.utility.error.keys.NoRollCallException
import io.reactivex.disposables.CompositeDisposable
import javax.inject.Inject
import timber.log.Timber

Expand Down Expand Up @@ -45,7 +46,7 @@ constructor(
linkedOrgRepo.otherLaoId != null) {
val laoId = context.channel.extractLaoId()
linkedOrgRepo.addLinkedLao(laoId, linkedOrgRepo.otherLaoId!!, arrayOf())
laoRepo.addDisposable(
disposables.add(
context.messageSender
.subscribe(Channel.getLaoChannel(linkedOrgRepo.otherLaoId!!))
.subscribe(
Expand Down Expand Up @@ -76,15 +77,15 @@ constructor(
// LAO. This might be changed in the future (making a pop-up asking the user if he/she wants
// to subscribe to that)
tokenExchange.tokens.forEach { t ->
laoRepo.addDisposable(
disposables.add(
Copy link
Contributor

Choose a reason for hiding this comment

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

Using the disposable from the handler doesn't give you the opportunity to clear the added disposables when closing a lao or the application. Take a look at the usage of disposables in the RollCallRepository or any other repository.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh ok, then should I create the disposables in LinkedOrganizationsRepository (as done in RollCallRepository) and then call it from the LinkedOrganizationsHandler ? Or should I do it directly in the handler ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep you can do it from the repository as for roll calls and keep it consistent

context.messageSender
.subscribe(
Channel.getLaoChannel(tokenExchange.laoId).subChannel(SOCIAL).subChannel(t))
.subscribe(
{ Timber.tag(TAG).d(SUCCESS) },
{ error: Throwable -> Timber.tag(TAG).e(error, ERROR) }))
}
laoRepo.addDisposable(
disposables.add(
context.messageSender
.subscribe(
Channel.getLaoChannel(tokenExchange.laoId).subChannel(SOCIAL).subChannel(REACTIONS))
Expand All @@ -106,6 +107,7 @@ constructor(

companion object {
private val TAG = LinkedOrganizationsHandler::class.java.simpleName
private val disposables = CompositeDisposable()
private const val SOCIAL = "social"
private const val REACTIONS = "reactions"
private const val SUCCESS = "subscription is a success"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,9 @@ class LinkedOrganizationsFragmentNonOrganizerTest {
@Inject
lateinit var laoRepository: LAORepository

@BindValue @Mock
lateinit var linkedOrganizationsViewModel: LinkedOrganizationsViewModel

@BindValue @Mock
lateinit var keyManager: KeyManager

Expand All @@ -54,6 +57,7 @@ class LinkedOrganizationsFragmentNonOrganizerTest {
laoRepository.updateLao(LAO)
Mockito.`when`(keyManager.mainKeyPair).thenReturn(KEY_PAIR)
Mockito.`when`(keyManager.mainPublicKey).thenReturn(POP_TOKEN.publicKey)
Mockito.`when`(linkedOrganizationsViewModel.getLinkedLaosMap()).thenReturn(mutableMapOf(LAO_ID to arrayOf()))
}
}

Expand All @@ -75,6 +79,18 @@ class LinkedOrganizationsFragmentNonOrganizerTest {
.check(matches(withEffectiveVisibility(Visibility.GONE)))
}

@Test
fun testLAOTextDisplayed() {
LinkedOrganizationsFragmentPageObject.listOrganizationsText().check(matches(isDisplayed()))
LinkedOrganizationsFragmentPageObject.noOrganizationsText().check(matches(withEffectiveVisibility(Visibility.GONE)))
}

@Test
fun testLAOTextIsCorrect() {
val validText = "List of linked organizations :\n\n$LAO_ID"
LinkedOrganizationsFragmentPageObject.listOrganizationsText().check(matches(withText(validText)))
}

companion object {
private val KEY_PAIR = Base64DataUtils.generateKeyPair()
private val POP_TOKEN = Base64DataUtils.generatePoPToken()
Expand Down
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add a test when also there are some organizations (so not just the empty case)?

Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ class LinkedOrganizationsFragmentOrganizerTest {
}

@Test
fun testTextDisplayed() {
fun testNoLAOTextDisplayed() {
LinkedOrganizationsFragmentPageObject.noOrganizationsText().check(matches(isDisplayed()))
LinkedOrganizationsFragmentPageObject.listOrganizationsText().check(matches(withEffectiveVisibility(Visibility.GONE)))
}
Expand Down
Loading