Louvorg/ReaderForSelfoss-multiplatform
Louvorg/ReaderForSelfoss-multiplatform
3
2
Fork 2
Code Issues 16 Pull Requests Actions Projects 1 Releases 68 Wiki Activity

Handle public instances #126

Merged
AmineB merged 8 commits from davidoskky/ReaderForSelfoss-multiplatform:public into master 2023-01-28 10:25:36 +00:00
Conversation 0 Commits 8 Files Changed 13 +5 -2
3 changed files with 5 additions and 2 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit ec0fd2faee - Show all commits

2
androidApp/src/main/java/bou/amine/apps/readerforselfossv2/android/ReaderActivity.kt
View File

@ -84,7 +84,7 @@ class ReaderActivity : AppCompatActivity(), DIAware {
}
private fun readItem(item: SelfossModel.Item) {
if (appSettingsService.isMarkOnScrollEnabled() and !appSettingsService.getPublicAccess()) {
if (appSettingsService.isMarkOnScrollEnabled() && !appSettingsService.getPublicAccess()) {
AmineB commented 2023-01-26 19:15:18 +00:00
Outdated
Review
Copy Link

and should be changed to &&.

We do not care about the value of appSettingsService.getPublicAccess() if appSettingsService.isMarkOnScrollEnabled() is false.

`and` should be changed to `&&`. We do not care about the value of `appSettingsService.getPublicAccess()` if `appSettingsService.isMarkOnScrollEnabled()` is false.
👍 1
CoroutineScope(Dispatchers.IO).launch {
repository.markAsRead(item)
}

2
shared/src/commonMain/kotlin/bou/amine/apps/readerforselfossv2/model/SelfossModel.kt
View File

@ -51,7 +51,7 @@ class SelfossModel {
fun getApiConfiguration() = configuration ?: ApiConfiguration(null, null)
}
@kotlinx.serialization.Serializable
@Serializable
AmineB commented 2023-01-26 19:24:24 +00:00
Outdated
Review
Copy Link

Can be changed to @Serializable

Can be changed to `@Serializable`
👍 1
data class ApiConfiguration(
@Serializable(with = BooleanSerializer::class)
val publicMode: Boolean?,

3
shared/src/commonMain/kotlin/bou/amine/apps/readerforselfossv2/repository/RepositoryImpl.kt
View File

@ -448,6 +448,9 @@ class Repository(
if (fetchedInformation.data.getApiMajorVersion() != apiMajorVersion) {
appSettingsService.updateApiVersion(fetchedInformation.data.getApiMajorVersion())
}
// Check if we're accessing the instance in public mode
// This happens when auth and public mode are enabled but
// no credentials are provided to login
AmineB commented 2023-01-26 19:35:47 +00:00
Outdated
Review
Copy Link

I didn't look into how public instances work, but this condition seems weird.

isAuthEnabled and isPublicModeEnabled can be true at the same time ?

They seem to mean different things.

I didn't look into how public instances work, but this condition seems weird. `isAuthEnabled` and `isPublicModeEnabled` can be `true` at the same time ? They seem to mean different things.
davidoskky commented 2023-01-26 20:51:47 +00:00
Outdated
Review
Copy Link

Yes, it is the normal case in fact; the weird condition is actually isAuthEnabled false and isPublicModeEnabled true.
Authentication is enabled because the administrator user can log in and read/favourite articles or edit sources while everyone else can just read the articles (and not mark them as read). Having authentication disabled simply means that no password is required to access and thus everyone is administrator, in this case it would make no sense to enable public mode as well.

Yes, it is the normal case in fact; the weird condition is actually `isAuthEnabled` `false` and `isPublicModeEnabled` `true`. Authentication is enabled because the administrator user can log in and read/favourite articles or edit sources while everyone else can just read the articles (and not mark them as read). Having authentication disabled simply means that no password is required to access and thus everyone is administrator, in this case it would make no sense to enable public mode as well.
AmineB commented 2023-01-26 20:56:19 +00:00
Outdated
Review
Copy Link

Can you please add this to a comment ?

Can you please add this to a comment ?
👍 1
if (appSettingsService.getUserName().isEmpty()
AmineB commented 2023-01-26 19:36:46 +00:00
Review
Copy Link

appSettingsService.updatePublicAccess(fetchedInformation.data.getApiConfiguration().isPublicModeEnabled()) isn't enough ?

`appSettingsService.updatePublicAccess(fetchedInformation.data.getApiConfiguration().isPublicModeEnabled())` isn't enough ?
davidoskky commented 2023-01-26 20:54:18 +00:00
Review
Copy Link

No, public mode might be enabled but we could still log into the instance with username and password.
Public mode has limitations, thus if one had the credentials to access the instance, that would actually be the preferred option.

No, public mode might be enabled but we could still log into the instance with username and password. Public mode has limitations, thus if one had the credentials to access the instance, that would actually be the preferred option.
davidoskky commented 2023-01-27 13:01:54 +00:00
Review
Copy Link

I'll rectify to explain what I mean. In the settings I am not storing the remote value of the configuration, I am directly storing whether we connected through public mode or not; in this way in any point of the application in which this information is required, that'll be readily available without any further processing. That's because we could connect trough the normal authentication method even if public mode was enabled.
Alternatively, I'd have to store both values of the remote configuration: isPublicModeEnabled and isAuthEnabled and each time check if these imply that we're connecting through public mode.
Since we are not using these information for anything else, storing it would be redundant.

I'll rectify to explain what I mean. In the settings I am not storing the remote value of the configuration, I am directly storing whether we connected through public mode or not; in this way in any point of the application in which this information is required, that'll be readily available without any further processing. That's because we could connect trough the normal authentication method even if public mode was enabled. Alternatively, I'd have to store both values of the remote configuration: `isPublicModeEnabled` and `isAuthEnabled` and each time check if these imply that we're connecting through public mode. Since we are not using these information for anything else, storing it would be redundant.
&& fetchedInformation.data.getApiConfiguration().isAuthEnabled()
&& fetchedInformation.data.getApiConfiguration().isPublicModeEnabled()) {