Skip to content

Show file download/upload/print affordances in app context#15050

Open
rtibblesbot wants to merge 1 commit into
learningequality:developfrom
rtibblesbot:issue-14654-9ea726
Open

Show file download/upload/print affordances in app context#15050
rtibblesbot wants to merge 1 commit into
learningequality:developfrom
rtibblesbot:issue-14654-9ea726

Conversation

@rtibblesbot

@rtibblesbot rtibblesbot commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Summary

  • isAppContext gates hid file upload/download/print affordances in installed apps — a workaround from when apps couldn't handle downloads.
  • Windows/Mac handle downloads natively; Android WebView support is trivial. Removes the four gates so the affordances render in every context.
  • Affordances: content "Save to device", coach CSV export, facility CSV import/export + log downloads, "Print all passwords".

References

Fixes #14654. Android WebView download support: learningequality/kolibri-installer-android#267.

Reviewer guidance

Confirm each of:

  • Printing
  • File upload (e.g. user import)
  • File download (e.g. log file download)

On each of:

  • Android app
  • Mac App
  • Windows App

AI usage

Used Claude Code to remove the gates and update tests following a pre-approved plan. Verified with the affected Jest suites and prek lint.


@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly

How was this generated?
  • Ran pre-flight CI checks (lint, format, tests) and verified all pass
  • Rebased onto the target branch and resolved any conflicts
  • Reorganized commit history into clean, logical commits
  • Audited the diff to ensure only issue-relevant files are changed
  • Built PR body from the repository's PR template with evidence blocks
@rtibblesbot

🟡 Waiting for feedback

Last updated: 2026-07-24 03:27 UTC

@github-actions github-actions Bot added APP: Facility Re: Facility App (user/class management, facility settings, csv import/export, etc.) APP: Coach Re: Coach App (lessons, quizzes, groups, reports, etc.) DEV: frontend SIZE: small labels Jul 23, 2026
@github-actions

Copy link
Copy Markdown
Contributor

npm Package Versions

Warning

The following packages have changed files but no version bump:

Package Version Changed files
kolibri 0.18.0 2

If these changes affect published code, consider bumping the version.

@github-actions

github-actions Bot commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

@rtibblesbot
rtibblesbot marked this pull request as ready for review July 23, 2026 03:29

@rtibbles rtibbles left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A little bit more cleanup, but the code here looks broadly sensible.

@@ -85,8 +78,7 @@
computed: {
...mapState('classSummary', ['learnerMap']),
exportDisabled() {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we could go a step further and instead just use "disableExport" prop anywhere "exportDisabled" is used now, and cut out the indirection?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good call — done in 01a946c. Removed the exportDisabled pass-through computed and used disableExport directly in the template v-if.

},
canDownload() {
return !this.isAppContext && this.downloadableFiles.length;
return this.downloadableFiles.length;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Worth coercing to a Boolean?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done in 01a946ccanDownload now returns this.downloadableFiles.length > 0.

@rtibbles rtibbles left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Just one more bit of cleanup, then please tidy up the commit history.

});

it('does not render if isAppContext is true', () => {
it('renders in app context when there are renderable document files', () => {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we can just remove this test - it was previously asserting this because of a special case, which no longer exists.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done — removed in 73c49d2. With the isAppContext gate gone, canDownload no longer special-cases app context, so this test was just a duplicate of "should render if isAppContext is false and there are renderable document files".

@rtibbles rtibbles left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Code makes sense - needs commit history tidy up, ready for manual QA though.

@rtibblesbot

Copy link
Copy Markdown
Contributor Author

Tidied the commit history — squashed the four per-component commits into a single commit (Show file upload/download/print affordances in app context) since this is one cohesive change removing the isAppContext gates. The tree is byte-identical to the previous head; only the history changed. Ready for manual QA.

Several file upload/download/print UI affordances were gated behind
isAppContext, hiding them in the installed apps. That was a workaround
for the assumption that apps could not handle file downloads; the
Windows and Mac apps handle this natively and the Android WebView work
is tracked in kolibri-installer-android#267. Remove the gates so the
affordances render regardless of context:

- DownloadButton "Save to device" button visibility no longer checks
  isAppContext.
- ReportsControls coach report CSV export drops isAppContext from
  exportDisabled (and the stale "disable in app mode" comment).
- DataPage facility CSV import/export and session/summary log download
  sections drop the canUploadDownloadFiles gate.
- AllPasswordsPage "Print all passwords" button always renders and the
  title row always uses the two-column layout, on both the coach and
  facility passwords pages.

Behaviour in non-app context is unchanged. Blocking Jest tests are
removed and replaced with app-context render assertions.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

APP: Coach Re: Coach App (lessons, quizzes, groups, reports, etc.) APP: Facility Re: Facility App (user/class management, facility settings, csv import/export, etc.) DEV: frontend SIZE: small

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remove isAppContext gates on file upload/download capabilities

2 participants