Skip to content

Subscriber sort order #21995

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Jul 8, 2025
Merged

Subscriber sort order #21995

merged 7 commits into from
Jul 8, 2025

Conversation

nbradbury
Copy link
Contributor

@nbradbury nbradbury commented Jul 7, 2025

This PR updates the subscriber list by adding ascending & descending items to the sort dropdown menu. I also removed "Plan" as a sort field to match iOS.

To test:

  • My Site > Subscribers
  • Tap the sort icon
  • Note the addition of ascending & descending items
  • Note that changing the sort order works as expected
sort.mp4

@nbradbury nbradbury added unit-tests-exemption Subscribers View and manage newsletter subscribers labels Jul 7, 2025
@nbradbury nbradbury requested a review from Copilot July 7, 2025 18:57
@dangermattic
Copy link
Collaborator

dangermattic commented Jul 7, 2025

2 Warnings
⚠️ strings.xml files should only be updated on release branches, when the translations are downloaded by our automation.
⚠️ PR is not assigned to a milestone.

Generated by 🚫 Danger

Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds ascending and descending sort order options to the subscriber list dropdown and wires them through the view models and UI layers.

  • Introduces two new string resources for “Ascending” and “Descending.”
  • Extends DataViewViewModel and SubscribersViewModel to track and apply sort order.
  • Updates SubscribersActivity and DataViewScreen composables to render and handle the new sort-order menu items.

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
strings.xml Added string resources for “ascending” and “descending.”
SubscribersViewModel.kt Implemented getDefaultSort() override.
SubscribersActivity.kt Passed currentSortOrder and onSortOrderClick to the UI.
DataViewViewModel.kt Introduced _sortOrder state, sortOrder flow, and onSortOrderClick.
DataViewScreen.kt Created SortDropdownMenuButton with ascending/descending items and updated previews.
Comments suppressed due to low confidence (2)

WordPress/src/main/java/org/wordpress/android/ui/dataview/DataViewViewModel.kt:189

  • The new onSortOrderClick logic and the associated sortOrder state changes aren't covered by existing unit tests. Consider adding tests to verify state updates and that fetchData() is called.
    fun onSortOrderClick(order: WpApiParamOrder) {

WordPress/src/main/java/org/wordpress/android/ui/dataview/DataViewScreen.kt:318

  • In the descending DropdownMenuItem handler, ensure that menuExpanded = false is executed inside the onClick lambda so the menu closes after selection, matching the ascending behavior.
                onClick = {

Comment on lines +212 to 249
* Dropdown menu button for displaying a list of filter [DataViewDropdownItem]s
*/
@Composable
private fun FilterDropdownMenuButton(
filters: List<DataViewDropdownItem>,
currentFilter: DataViewDropdownItem?,
onFilterClick: (DataViewDropdownItem) -> Unit,
) {
var menuExpanded by remember { mutableStateOf(false) }
IconButton(
onClick = {
menuExpanded = !menuExpanded
},
modifier = Modifier
.size(48.dp)
) {
Icon(
imageVector = ImageVector.vectorResource(id = R.drawable.ic_filter_list_white_24dp),
contentDescription = stringResource(id = R.string.filter),
tint = MaterialTheme.colorScheme.primary
)
DropdownMenu(
expanded = menuExpanded,
onDismissRequest = {
menuExpanded = false
}
) {
DropdownItems(
titleRes = R.string.filter,
items = filters,
currentItem = currentFilter,
onItemClick = { item ->
onSortClick(item)
onFilterClick(item)
menuExpanded = false
}
)
}
}

This comment was marked as resolved.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Jul 7, 2025

Jetpack📲 You can test the changes from this Pull Request in Jetpack by scanning the QR code below to install the corresponding build.
App NameJetpack Jetpack
FlavorJalapeno
Build TypeDebug
Versionpr21995-c437bcf
Commitc437bcf
Direct Downloadjetpack-prototype-build-pr21995-c437bcf.apk
Note: Google Login is not supported on these builds.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Jul 7, 2025

WordPress📲 You can test the changes from this Pull Request in WordPress by scanning the QR code below to install the corresponding build.
App NameWordPress WordPress
FlavorJalapeno
Build TypeDebug
Versionpr21995-c437bcf
Commitc437bcf
Direct Downloadwordpress-prototype-build-pr21995-c437bcf.apk
Note: Google Login is not supported on these builds.

Copy link

codecov bot commented Jul 7, 2025

Codecov Report

Attention: Patch coverage is 0% with 69 lines in your changes missing coverage. Please review.

Project coverage is 39.02%. Comparing base (a8cfad9) to head (c437bcf).
Report is 2 commits behind head on trunk.

Files with missing lines Patch % Lines
...rg/wordpress/android/ui/dataview/DataViewScreen.kt 0.00% 63 Missing ⚠️
...wordpress/android/ui/dataview/DataViewViewModel.kt 0.00% 6 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##            trunk   #21995   +/-   ##
=======================================
  Coverage   39.02%   39.02%           
=======================================
  Files        2153     2153           
  Lines      101494   101494           
  Branches    15585    15585           
=======================================
  Hits        39613    39613           
  Misses      58384    58384           
  Partials     3497     3497           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@nbradbury nbradbury marked this pull request as ready for review July 7, 2025 19:59
@nbradbury nbradbury requested a review from adalpari July 7, 2025 19:59
Copy link

sonarqubecloud bot commented Jul 8, 2025

Copy link
Contributor

@adalpari adalpari left a comment

Choose a reason for hiding this comment

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

Looks good!!

Just as a suggestion, what about start adding tests for DataViewViewModel so we prevent breaking the current stuff while adding all these new changes?

@nbradbury
Copy link
Contributor Author

what about start adding tests for DataViewViewModel so we prevent breaking the current stuff while adding all these new changes?

I was planning on making tests one of the last things I do, simply because I expect to make changes to that view model as I use it more. But if you think it's important now, I can create the tests in a separate PR.

@adalpari
Copy link
Contributor

adalpari commented Jul 8, 2025

what about start adding tests for DataViewViewModel so we prevent breaking the current stuff while adding all these new changes?

I was planning on making tests one of the last things I do, simply because I expect to make changes to that view model as I use it more. But if you think it's important now, I can create the tests in a separate PR.

Fair enough. It was just a comment. So, feel free to stick with your plan :)

@nbradbury nbradbury merged commit 2927acb into trunk Jul 8, 2025
26 checks passed
@nbradbury nbradbury deleted the feature/subscriber-sort-order branch July 8, 2025 14:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Subscribers View and manage newsletter subscribers unit-tests-exemption
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants