Skip to content
This repository has been archived by the owner on Feb 20, 2023. It is now read-only.

For #15931: Sort Downloads from newest to oldest #15939

Merged

Conversation

joc-a
Copy link
Contributor

@joc-a joc-a commented Oct 15, 2020

Pull Request checklist

  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not
  • Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features. In addition, it includes a screenshot of a successful accessibility scan to ensure no new defects are added to the product.

To download an APK when reviewing a PR:

  1. click on Show All Checks,
  2. click Details next to "Taskcluster (pull_request)" after it appears and then finishes with a green checkmark,
  3. click on the "Fenix - assemble" task, then click "Run Artifacts".
  4. the APK links should be on the left side of the screen, named for each CPU architecture

Downloads are now sorted from newest to oldest based on createdTime.

This is how the downloads appear in the notifications on the phone (sorted from newest to oldest):
Screenshot_1603711537

This is how the downloads now appear on Fenix (the same order as above):
Screenshot_1603711558

@joc-a joc-a requested review from a team as code owners October 15, 2020 21:07
@firefoxci-taskcluster
Copy link

No Taskcluster jobs started for this pull request
The `allowPullRequests` configuration for this repository (in `.taskcluster.yml` on the
default branch) does not allow starting tasks for this pull request.

@gabrielluong
Copy link
Member

@joc-a Thank you for the contribution! Would you be able to post some screenshots for a ux-review?

@gabrielluong gabrielluong added needs:review PRs that need to be reviewed needs:UX-feedback Needs UX Feedback labels Oct 15, 2020
Copy link
Contributor

@Amejia481 Amejia481 left a comment

Choose a reason for hiding this comment

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

Thanks for the patch 👍🏽
It looks great, a left one comment before approving ✅

it.status == DownloadState.Status.COMPLETED
}.filterNotExistsOnDisk()
val items = requireComponents.core.store.state.downloads.values
.sortedByDescending { it.createdTime } // sort from newest to oldest
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we add a test to cover the new feature? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure! I'll try my hand at writing a test.

Copy link
Contributor Author

@joc-a joc-a Oct 16, 2020

Choose a reason for hiding this comment

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

I don't have much experience writing tests, but I thought I'd give it a shot.

I added createdTime field to DownloadItem and tried with the following code I wrote in DownloadControllerTest:

@Test
fun `downloads are sorted from newest to oldest`() {
        val testList = state.items

        val expectedList = testList.sortedByDescending { it.createdTime }

        assertEquals(testList, expectedList)
}

But that didn't work. I'm very new to this so maybe I'm not approaching this the right way, but I'd be happy to get some guidance! 😊

Copy link
Contributor

@Amejia481 Amejia481 Oct 19, 2020

Choose a reason for hiding this comment

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

@joc-a here you can see an example. I would modify a bit DownloadFragment to make it testable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Amejia481 Done, thanks so much!

@kglazko
Copy link
Contributor

kglazko commented Oct 15, 2020

Can we wait to land this? This could potentially cause conflicts with the large Delete Downloads PR I am wrapping up.

@gabrielluong gabrielluong added pr:needs-changes PRs that need some changes/fixes before they can land and removed needs:review PRs that need to be reviewed labels Oct 15, 2020
@codecov-io
Copy link

codecov-io commented Oct 16, 2020

Codecov Report

Merging #15939 into master will increase coverage by 0.08%.
The diff coverage is 52.17%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #15939      +/-   ##
============================================
+ Coverage     29.68%   29.77%   +0.08%     
  Complexity     1195     1195              
============================================
  Files           453      453              
  Lines         18555    18535      -20     
  Branches       2544     2536       -8     
============================================
+ Hits           5508     5518      +10     
+ Misses        12625    12594      -31     
- Partials        422      423       +1     
Impacted Files Coverage Δ Complexity Δ
...pp/src/main/java/org/mozilla/fenix/FeatureFlags.kt 100.00% <ø> (ø) 5.00 <0.00> (-1.00)
...a/org/mozilla/fenix/browser/BaseBrowserFragment.kt 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
.../java/org/mozilla/fenix/browser/BrowserAnimator.kt 14.28% <0.00%> (+2.52%) 0.00 <0.00> (ø)
...src/main/java/org/mozilla/fenix/components/Core.kt 21.11% <0.00%> (-0.54%) 1.00 <0.00> (ø)
...g/mozilla/fenix/settings/SecretSettingsFragment.kt 0.00% <ø> (ø) 0.00 <0.00> (ø)
.../src/main/java/org/mozilla/fenix/utils/Settings.kt 69.65% <ø> (-0.25%) 35.00 <0.00> (ø)
...ozilla/fenix/library/downloads/DownloadFragment.kt 28.88% <85.71%> (+28.88%) 1.00 <1.00> (+1.00)
...a/fenix/library/downloads/DownloadFragmentStore.kt 42.10% <0.00%> (+5.26%) 0.00% <0.00%> (ø%)
...a/org/mozilla/fenix/library/LibraryPageFragment.kt 5.88% <0.00%> (+5.88%) 1.00% <0.00%> (+1.00%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6353e99...e362103. Read the comment docs.

@joc-a joc-a force-pushed the 15931_sort_downloads_from_new_to_old branch 2 times, most recently from a59dc6c to 78f6cf6 Compare October 21, 2020 16:07
@liuche
Copy link
Contributor

liuche commented Oct 22, 2020

If your changes addressed the comments, please tag the reviewer for another review!

In order for UX to sign off, please also add a screenshot.

Thanks for your help @joc-a !

sort downloads from newest to oldest based on createdTime
@joc-a joc-a force-pushed the 15931_sort_downloads_from_new_to_old branch from 78f6cf6 to bcc22fe Compare October 25, 2020 21:43
add a test to make sure downloads are sorted from newest to oldest
@joc-a joc-a force-pushed the 15931_sort_downloads_from_new_to_old branch from bcc22fe to e362103 Compare October 26, 2020 11:05
@joc-a joc-a requested a review from Amejia481 October 26, 2020 11:30
Copy link
Contributor

@Amejia481 Amejia481 left a comment

Choose a reason for hiding this comment

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

This looks good to me,
@kglazko do you still want to wait for your pr to land?

@Amejia481 Amejia481 added needs:review PRs that need to be reviewed and removed pr:needs-changes PRs that need some changes/fixes before they can land labels Oct 27, 2020
@gabrielluong gabrielluong removed the needs:review PRs that need to be reviewed label Oct 29, 2020
@gabrielluong gabrielluong added the pr:approved PR that has been approved label Oct 29, 2020
@gabrielluong gabrielluong merged commit 1b20415 into mozilla-mobile:master Nov 2, 2020
@joc-a joc-a deleted the 15931_sort_downloads_from_new_to_old branch November 2, 2020 16:13
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
needs:UX-feedback Needs UX Feedback pr:approved PR that has been approved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants