[MBL-19040][Teacher] Edit File page state sets back to default after orientation change#3684
[MBL-19040][Teacher] Edit File page state sets back to default after orientation change#3684kristofnemere wants to merge 1 commit into
Conversation
…orientation change Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
Nice refactor — moving the edit state (accessStatus, lockDate/unlockDate, usageType, licenseType, editedName, editedCopyright) out of the fragment and into the presenter is a clear win for surviving configuration changes (e.g., rotation), and it also makes the derivation logic far more testable. The diff is well-scoped and the call sites read cleanly.
A few things I especially liked:
- Fixing the missing
elseinsetupAccessso thatRestrictedScheduleStatus()is actually returned when lock/unlock dates exist (previously the expression was constructed and silently discarded). - Replacing
licenseList.filter { ... }[0]with.firstOrNull()to avoid a potentialIndexOutOfBoundsExceptionon an empty CC list. - Consolidating spinner initial-position logic into clean
when (presenter.accessStatus)blocks driven by sealed-class subtypes rather than re-readingcurrentFileOrFolderfields in multiple places. - Adding the
copyrightTextWatcherso the copyright field's state persists in the presenter alongside the title.
Issues to consider
- Behavior change:
lockDate/unlockDateare now seeded from the file unconditionally (presenter lines 45–46), including forisLockedfiles — previously they were only seeded for the hidden/scheduled branch. Combined with thesetupAccesselsefix, this changes the saved payload when a user switches a previously-locked file with dates back to "Restricted Access". (inline) -
setSelection(-1)edge case insetupLicenseswhen bothpresenter.licenseType?.nameis null and the filtered CC list is empty —getPosition(null)returns-1. Consider.coerceAtLeast(0). (inline) - Redundant assignment in
setupAccess'srestrictedAccessbranch — overwritten immediately bysetupRestrictedAccess's listener. Not a defect, just a cleanup opportunity. (inline) - No unit tests added for the new presenter state. Now that initialization is a pure function of
currentFileOrFolder, this is the perfect moment to add anEditFileFolderPresenterTestcovering the access-status precedence and field defaulting. (inline)
| var lockDate: Date? = currentFileOrFolder.lockDate | ||
| var unlockDate: Date? = currentFileOrFolder.unlockDate |
There was a problem hiding this comment.
Subtle behavior change worth verifying: previously the fragment's lockDate/unlockDate were only seeded from the file when it was hidden/scheduled (the isHidden || lockDate != null || unlockDate != null branch in the old setupAccess). Now they're seeded from the file unconditionally, including for isLocked (unpublished) files.
Combined with the bug fix in setupAccess (the missing else between RestrictedScheduleStatus() and RestrictedStatus()), this means: if a user opens a locked file that previously had lock/unlock dates and switches the access spinner to "Restricted Access", the status will now resolve to RestrictedScheduleStatus() and the prior dates will be sent on save. Previously, switching always resolved to RestrictedStatus() (due to the missing else) and the dates were not used.
This is probably the intended improvement, but it does change the saved payload for that flow — please confirm it matches the desired UX.
| val initialPosition = spinnerAdapter.getPosition(presenter.licenseType?.name | ||
| ?: presenter.licenseList.filter { it.name.contains("CC") }.firstOrNull()?.name) |
There was a problem hiding this comment.
Nice safety improvement over the original [0] (which would throw IndexOutOfBoundsException on an empty CC license list). One edge case to consider though: if presenter.licenseType?.name is null and the filtered CC list is empty, firstOrNull()?.name returns null, and ArrayAdapter.getPosition(null) returns -1. licenseSpinner.setSelection(-1) will leave the spinner in an unselected state.
If licenseList is guaranteed to contain at least one CC entry, this is fine. Otherwise, consider defaulting to 0:
val initialPosition = spinnerAdapter.getPosition(presenter.licenseType?.name
?: presenter.licenseList.firstOrNull { it.name.contains("CC") }?.name)
.coerceAtLeast(0)| getString(R.string.restrictedAccess) -> { | ||
| if (lockDate != null || unlockDate != null) | ||
| RestrictedScheduleStatus() | ||
| RestrictedStatus() | ||
| if (presenter.lockDate != null || presenter.unlockDate != null) RestrictedScheduleStatus() | ||
| else RestrictedStatus() | ||
| } |
There was a problem hiding this comment.
Good catch fixing the missing else here — the original if (lockDate != null || unlockDate != null) RestrictedScheduleStatus() followed by RestrictedStatus() always returned RestrictedStatus() regardless of the dates because the RestrictedScheduleStatus() expression was constructed and discarded.
Note that this assignment is somewhat redundant: setupRestrictedAccess runs immediately after setupAccess and its onItemSelected listener also writes to presenter.accessStatus whenever the restricted access section becomes visible — so this value ends up being overwritten. Not a defect, just worth being aware of if you later refactor further.
| var accessStatus: FileAccessStatus = when { | ||
| currentFileOrFolder.isLocked -> UnpublishStatus() | ||
| currentFileOrFolder.isHidden -> RestrictedStatus() | ||
| currentFileOrFolder.unlockDate != null || currentFileOrFolder.lockDate != null -> RestrictedScheduleStatus() | ||
| else -> PublishStatus() | ||
| } |
There was a problem hiding this comment.
Now that the access-status derivation lives in the presenter (a pure function of currentFileOrFolder) it would be a great candidate for a small unit test. I don't see an EditFileFolderPresenterTest in apps/teacher/src/test/. Suggested cases:
isLocked = true→UnpublishStatusisHidden = true→RestrictedStatuslockDate != null(only) →RestrictedScheduleStatusunlockDate != null(only) →RestrictedScheduleStatus- neither →
PublishStatus isLocked && isHidden(precedence) →UnpublishStatusisHidden && lockDate != null(precedence) →RestrictedStatus
Also worth covering editedName fallback to displayName when name is null, and editedCopyright defaulting to empty when usageRights is null — these previously lived in the fragment and were untestable, so this is a nice opportunity to lock the behavior in.
🧪 Unit Test Results✅ 📱 Teacher App
✅ 📦 Submodules
📊 Summary
Last updated: Mon, 11 May 2026 13:43:26 GMT |
📊 Code Coverage Report✅ Student
|
|
Teacher Install Page |
3 participants
Test plan:
refs: MBL-19040
affects: Teacher
release note: Fixed an issue where the Edit File screen lost its in-progress changes after rotating the device.
Checklist