Dev schoolconfing#180
Conversation
| interface SchoolConfigSettingEntityDao { | ||
|
|
||
| @Insert(onConflict = OnConflictStrategy.REPLACE) | ||
| suspend fun insert(entity: SchoolConfigSettingEntity) |
There was a problem hiding this comment.
Do we need this and the insertlist function?
| } | ||
| } | ||
|
|
||
| override fun listAsPagingSource( |
There was a problem hiding this comment.
I don't think we need the paging source. We are never using the school configuration to show a long list that the user scrolls through. Please remove listAsPagingSource.
mikedawson
left a comment
mikedawson
left a comment
There was a problem hiding this comment.
Hello @Anugraha-sutara - this looks quite good to me. The permission checks look right. The migration also looks good.
One important improvement request: the DAO has queries for both multiple keys and a single key function. This seems like duplication to me.
I think the datasource needs to support multiple keys in a single query: e.g. for shared school device management, the screen needs to get both the setting for the PIN and the setting for whether or not user self-selection is enabled. This should not require two http requests.
Please update the datasource list params so that it can include a list of keys. This would work whether a viewmodel needs one key or more than one.
then please remove Dao functions that are no longer needed. Thanks.
…eys instead of a single key
mikedawson
left a comment
mikedawson
left a comment
There was a problem hiding this comment.
Please put the test for school config here in the school config branch such that it can be merged.
| return GetListParams( | ||
| common = GetListCommonParams.fromParams(params) | ||
| common = GetListCommonParams.fromParams(params), | ||
| keys = params.getAll(DataLayerParams.KEYS) ?: params[DataLayerParams.KEY]?.let { listOf(it) } |
There was a problem hiding this comment.
No need for "key" on it's own. Please test to ensure that conversion to/from parameters works as expected.
If it works as-is, it's OK.
There is no need for a special case for list size = 1.
mikedawson
left a comment
mikedawson
left a comment
There was a problem hiding this comment.
The current test does not really test the repository or client server communication (as per the reference cited). Please update it to ensure it actually uses the repository, not using updateLocal as a 'cheat'.
2 participants
Implement: SchoolConfigSettingDataSource for db, http, and repository