Allow a post to be marked as sticky #15351

dnalves · 2021-09-18T17:38:56Z

This is still in progress, please don't merge

Description

We wanna allow the users to mark a post as sticky from the Android app. This is already present in iOS and Web.
A blocker for this PR is the approval of wordpress-mobile/WordPress-FluxC-Android#2124 and an update of the FluxC library (which will add the "sticky" field to the PostModel).

Work done

A switch was added to Post Settings screen. This is loaded and updated in the PostModel (from FluxC), which syncs it to the Wordpress servers.
This switch only appears for Posts, so it is hidden for Pages.

Screenshot

To be done

Identify any regression areas
Localize strings
Add details to the release-notes

To test:

Open a site with posts in it
Open a post to edit it
Tap to edit "Post settings"
Toggle the switch "Stick post to the front page"
Update the post with the changes (sync it)
Check on Web or iOS that the changes were applied
Revert the change on Web or iOS
Check on Android that the change is reflected there

Regression Notes

Potential unintended areas of impact
None
What I did to test those areas of impact (or what existing automated tests I relied on)
Manual tests
What automated tests I added (or what prevented me from doing so)
There are no tests surrounding that Fragment. And any unit tests added will be testing the FluxC library.

PR submission checklist:

I have completed the Regression Notes.
I have considered adding accessibility improvements for my changes.
I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

For now, it's still a dummy switch (due to FluxC needing to be updated). We're updating its value when all the views are updated.

This depends on an update of the FluxC library (PR pending).

Added "Allow users to mark Posts as sticky [wordpress-mobile#15351]"

peril-wordpress-mobile · 2021-09-21T03:08:06Z

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

peril-wordpress-mobile · 2021-09-21T03:25:05Z

You can test the changes on this Pull Request by downloading the APKs:

WordPress/src/main/java/org/wordpress/android/ui/posts/EditPostSettingsFragment.java

WordPress/src/main/res/layout/edit_post_settings_fragment.xml

peril-wordpress-mobile · 2021-09-21T11:55:14Z

	Warnings
⚠️	PR is not assigned to a milestone.

Generated by 🚫 dangerJS

jd-alexander

The FluxC component of this PR has been merged in. A FluxC artifact is being built on FluxC's develop wordpress-mobile/WordPress-FluxC-Android@e20192c

jd-alexander

@dnalves while testing and comparing both platforms I noticed that on iOS we are showing a Sticky label on the Posts List Cell as seen below in the screenshot. This was not included in the design specs that were attached to the issue. Nonetheless, in the name of platform synchronicity, we could make both Posts Lists Item States represent the same behavior when a post is sticky. What do you think?

jd-alexander · 2021-09-21T18:45:31Z

@dnalves while testing and comparing both platforms I noticed that on iOS we are showing a Sticky label on the Posts List Cell as seen below in the screenshot. This was not included in the design specs that were attached to the issue. Nonetheless, in the name of platform synchronicity, we could make both Posts Lists Item States represent the same behavior when a post is sticky. What do you think?

Just a note here, from a design standpoint, if you agree with proceeding we could replicate this design approach i.e putting the label in a separate row within the Posts List Item as a first pass while we inquire about design feedback.

dnalves · 2021-09-21T18:51:45Z

Nice catch @jd-alexander, it makes total sense for the behavior to be similar.

From what I noticed, the "Sticky" label is concatenated with the other labels at the bottom. We can replicate it. What do you think?

Android	iOS

jd-alexander · 2021-09-21T19:07:43Z

Nice catch @jd-alexander, it makes total sense for the behavior to be similar.

I agree!

From what I noticed, the "Sticky" label is concatenated with the other labels at the bottom. We can replicate it. What do you think?

Good observation! Sounds like an optimal approach that adheres to the current design specifications of the Posts List Item! I will check it out.

jd-alexander · 2021-09-22T04:29:31Z

WordPress/src/main/java/org/wordpress/android/viewmodel/posts/PostListItemUiStateHelper.kt

@@ -3,6 +3,7 @@ package org.wordpress.android.viewmodel.posts
 import org.apache.commons.text.StringEscapeUtils
 import org.wordpress.android.BuildConfig
 import org.wordpress.android.R
+import org.wordpress.android.R.string


I am not sure if we should be making this change 🤔 I know it is used in other places within the codebase but it seems unrelated to the task at hand here. If it's a case where the CI is failing due to it then I guess it's fine to proceed as it might be a new rule I am not aware of. Let me know your thoughts.

Android Studio function extraction did this and I didn't notice. When I read your comment I was surprised by it as well. I'm fixing this!

jd-alexander · 2021-09-22T04:33:03Z

WordPress/src/main/java/org/wordpress/android/viewmodel/posts/PostListItemUiStateHelper.kt

        }
+    }
+
+    @Suppress("LongParameterList")


I know this solution works. I have seen it in usage elsewhere but I know we are not fond of doing suppressions. Part of me feels it would be better if we simply pass the PostModel here and create these values within the function to lessen the parameters substantially. Do you have any thoughts on this discourse?

I'm not a fan as well, to be honest. I added here because I assumed in this case the long parameter list wasn't making the function more complex or harder to understand (given that the parameters were well named).

But I like your solution 😄 I'll pass the PostModel instead of a few parameters.

Thanks! I remember one of the concerns for passing around thePostModel is that it's quite a big object that contains so much information about the post that we may not require. To ensure we are making the best decision here, I am going to loop in another Android developer who has worked with these kind of decisions.

Hi @ParaskP7 I remember us making a decision about @Suppress("LongParameterList") in one of my PRs a few months back. In this case, we were opting to remove it and pass in a PostModel to represent several parameters that were extracted from it. However, we know the PostModel is quite a large object. Do you have any thoughts on this? Let me know. Thank you.

👋 @jd-alexander @dnalves !

I remember one of the concerns for passing around thePostModel is that it's quite a big object that contains so much information about the post that we may not require.
In this case, we were opting to remove it and pass in a PostModel to represent several parameters that were extracted from it. However, we know the PostModel is quite a large object.

Thanks for looping me in to this conversation Joel. I would personally go with passing the whole PostModel and thus resolving the LongParameterList warning. My reasoning would be:

The PostModel is already in memory, so I am not sure if it being a large object would degrade performance or cause any trouble. I haven't looked at the code extensively, but are you having concerns about passing this object between screens, thus having is unnecessarily being serialized and deserialised, or is it just being passed from one class/method to another?

I am not sure how big could potentially the PostModel end up being, thus I might be totally off here, but I would not try to optimize performance preemptively by sending individual properties of this object. However, this is my personal rule of thump, which might very well not apply here, so please take it with a grain of salt.

Let me know if I can further help here or whether I am missing some more context. 🙏

👋 @ParaskP7

Thanks for the quick response on this matter 😄 I agree with your points. About your question:

[...] are you having concerns about passing this object between screens, thus having is unnecessarily being serialized and deserialised, or is it just being passed from one class/method to another?

This is just from one method to another, so no serialization occurring in this case.

@jd-alexander: I made those changes in 982bd0f, already resolving the LongParameterList 😄

The PostModel is already in memory, so I am not sure if it being a large object would degrade performance or cause any trouble. I haven't looked at the code extensively, but are you having concerns about passing this object between screens, thus having is unnecessarily being serialized and deserialised, or is it just being passed from one class/method to another?

It's just being passed to another method and yes it is already in memory :)

I am not sure how big could potentially the PostModel end up being, thus I might be totally off here, but I would not try to optimize performance preemptively by sending individual properties of this object. However, this is my personal rule of thumb, which might very well not apply here, so please take it with a grain of salt.

Let me know if I can further help here or whether I am missing some more context. 🙏

The thoughts you shared were spot on. You picked up the context quite quickly, just like an Android API using requireContext() 😄 😆

Thanks for chiming in @ParaskP7 . I think the current approach here is fine!

Change which import we're using for R.string. Remove a detekt supression and fix the LongParameterList issue

jd-alexander · 2021-09-23T19:00:09Z

@dnalves I was doing some more testing and I noticed something.

If you create a post and you mark it as Sticky the list item displays Sticky in black color.
If you create a post and you mark it as Sticky and Private, the list item displays it in yellow color. If you remove the Private status, it causes an inconsistency because the Sticky label still remains yellow and others look black. What do you think about making it yellow by default? I think all labels have a color on Android that is not black.

jd-alexander · 2021-09-23T19:12:07Z

@dnalves since there were some new changes in FluxC we can use the latest develop hash before merging these changes. The changes made there are related to WooCommerce, so WP Android would be fine. Normally, we can merge both PRs when both are approved but it's also fine that it was merged in before since it doesn't cause any breaking changes if it were FluxC were to be updated in WP Android with another PR. Just noting that here :)

jd-alexander · 2021-09-24T00:15:37Z

WordPress/src/main/java/org/wordpress/android/viewmodel/posts/PostListItemUiStateHelper.kt

+        hasUnhandledConflicts: Boolean,
+        hasAutoSave: Boolean
+    ): List<UiString> {
+        val labels: MutableList<UiString> = getErrorAndProgressStatuses(


Isn't the type of labels inferred by the compiler? Let me know what you think.

Oh, nice catch! Android Studio method extraction tool made this and I missed it, thanks for pointing it out :D
985ed1f#diff-2238863dfadc135bc1b6359a21a855541d2dcf3d8cd2742d2fa91a81ea03e701L288

…tiple params

dnalves · 2021-09-24T08:59:03Z

@dnalves I was doing some more testing and I noticed something.

If you create a post and you mark it as Sticky the list item displays Sticky in black color.

If you create a post and you mark it as Sticky and Private, the list item displays it in yellow color. If you remove the Private status, it causes an inconsistency because the Sticky label still remains yellow and others look black. What do you think about making it yellow by default? I think all labels have a color on Android that is not black.

You're totally right! I missed this because I was testing only with private posts (which makes all the labels yellow). I added the "sticky" label to the list of "info" labels, which are colored yellow.

9f8175b

…h-to-post-settings

jd-alexander · 2021-09-24T17:52:58Z

@dnalves I was doing some more testing and I noticed something.

If you create a post and you mark it as Sticky the list item displays Sticky in black color.

If you create a post and you mark it as Sticky and Private, the list item displays it in yellow color. If you remove the Private status, it causes an inconsistency because the Sticky label still remains yellow and others look black. What do you think about making it yellow by default? I think all labels have a color on Android that is not black.

You're totally right! I missed this because I was testing only with private posts (which makes all the labels yellow). I added the "sticky" label to the list of "info" labels, which are colored yellow.

9f8175b

Thanks for these changes @dnalves Could you please update the tests to represent this behavior as well. When modifying files in WP Android, it's always best to check if there are associated tests ( which you have done previously 😎 , so I am just noting here as a general reminder.) 👍🏾

I normally Command-click a class to verify if tests are associated or not.

dnalves · 2021-09-24T18:36:02Z

@dnalves I was doing some more testing and I noticed something.

If you create a post and you mark it as Sticky the list item displays Sticky in black color.

If you create a post and you mark it as Sticky and Private, the list item displays it in yellow color. If you remove the Private status, it causes an inconsistency because the Sticky label still remains yellow and others look black. What do you think about making it yellow by default? I think all labels have a color on Android that is not black.

You're totally right! I missed this because I was testing only with private posts (which makes all the labels yellow). I added the "sticky" label to the list of "info" labels, which are colored yellow.
9f8175b

Thanks for these changes @dnalves Could you please update the tests to represent this behavior as well. When modifying files in WP Android, it's always best to check if there are associated tests ( which you have done previously 😎 , so I am just noting here as a general reminder.) 👍🏾

I normally Command-click a class to verify if tests are associated or not.

😱
Thanks for letting me know, my bad.
I added the tests here: ff5191b

…h-to-post-settings

jd-alexander

Thanks for these changes @dnalves LGTM 🚢

dnalves added 3 commits September 18, 2021 19:09

Add a (dummy) Switch to toggle if the post is sticky in Post Settings

1b9b0ad

For now, it's still a dummy switch (due to FluxC needing to be updated). We're updating its value when all the views are updated.

[PostSettings] Add logic to the "sticky" switch to update the PostModel

11a960f

This depends on an update of the FluxC library (PR pending).

[PostSettings] Hide "sticky" switch when is a page (and not a post)

0d0c7c0

jd-alexander mentioned this pull request Sep 20, 2021

Update the PostModel table with the field "sticky" wordpress-mobile/WordPress-FluxC-Android#2124

Closed

dnalves added 2 commits September 20, 2021 10:09

Update RELEASE-NOTES

44eac7a

Added "Allow users to mark Posts as sticky [wordpress-mobile#15351]"

Update fluxCVersion to PR wordpress-mobile#2126

e1a896f

jd-alexander reviewed Sep 21, 2021

View reviewed changes

WordPress/src/main/java/org/wordpress/android/ui/posts/EditPostSettingsFragment.java Outdated Show resolved Hide resolved

jd-alexander reviewed Sep 21, 2021

View reviewed changes

WordPress/src/main/java/org/wordpress/android/ui/posts/EditPostSettingsFragment.java Outdated Show resolved Hide resolved

jd-alexander reviewed Sep 21, 2021

View reviewed changes

WordPress/src/main/res/layout/edit_post_settings_fragment.xml Outdated Show resolved Hide resolved

dnalves added 3 commits September 21, 2021 09:42

[PostSettings] Rename Sticky switch listener and extract styles

82abfb7

Merge branch 'develop' into add-sticky-switch-to-post-settings

41f52e0

Update RELEASE-NOTES.txt

8dc8842

dnalves changed the title ~~[WIP] Allow a post to be marked as sticky [DON'T MERGE]~~ Allow a post to be marked as sticky Sep 21, 2021

dnalves marked this pull request as ready for review September 21, 2021 10:31

Update build.gradle to point fluxCVersion to the correct PR-commit

b268a43

jd-alexander reviewed Sep 21, 2021

View reviewed changes

jd-alexander requested changes Sep 21, 2021

View reviewed changes

Change FluxC version to the latest develop

a69566f

Display 'Sticky' label in the Post List for sticky posts

043caa8

Fix detekt and klint for PostListItemUiStateHelper

b0a811f

jd-alexander reviewed Sep 22, 2021

View reviewed changes

A few code improvements in PostListItemUiStateHelper

982bd0f

Change which import we're using for R.string. Remove a detekt supression and fix the LongParameterList issue

jd-alexander reviewed Sep 24, 2021

View reviewed changes

dnalves added 2 commits September 24, 2021 10:41

Refactor PostPageListLabelColorUseCase to use the Post instead of mul…

985ed1f

…tiple params

Change color of the sticky label to same as other info labels (yellow)

9f8175b

dnalves added 2 commits September 24, 2021 11:19

Update FluxC version to the latest develop commit

5d5989c

Merge remote-tracking branch 'upstream/develop' into add-sticky-switc…

c2893e8

…h-to-post-settings

Add unit tests for Sticky label color use case

ff5191b

dnalves and others added 3 commits October 5, 2021 12:11

Merge remote-tracking branch 'upstream/develop' into add-sticky-switc…

283f040

…h-to-post-settings

Update FluxC version to the latest develop

c1a6a81

Merge branch 'develop' into add-sticky-switch-to-post-settings

71b9e4f

jd-alexander approved these changes Oct 6, 2021

View reviewed changes

jd-alexander merged commit f3519d9 into wordpress-mobile:develop Oct 6, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow a post to be marked as sticky #15351

Allow a post to be marked as sticky #15351

dnalves commented Sep 18, 2021 •

edited

peril-wordpress-mobile bot commented Sep 21, 2021 •

edited

peril-wordpress-mobile bot commented Sep 21, 2021 •

edited

peril-wordpress-mobile bot commented Sep 21, 2021

jd-alexander left a comment

jd-alexander left a comment

jd-alexander commented Sep 21, 2021

dnalves commented Sep 21, 2021

jd-alexander commented Sep 21, 2021

jd-alexander Sep 22, 2021

dnalves Sep 22, 2021

jd-alexander Sep 22, 2021

dnalves Sep 22, 2021

jd-alexander Sep 22, 2021

ParaskP7 Sep 23, 2021

dnalves Sep 23, 2021

jd-alexander Sep 23, 2021

jd-alexander commented Sep 23, 2021 •

edited

jd-alexander commented Sep 23, 2021

jd-alexander Sep 24, 2021

dnalves Sep 24, 2021 •

edited

dnalves commented Sep 24, 2021

jd-alexander commented Sep 24, 2021 •

edited

dnalves commented Sep 24, 2021

jd-alexander left a comment

Allow a post to be marked as sticky #15351

Allow a post to be marked as sticky #15351

Conversation

dnalves commented Sep 18, 2021 • edited

Description

Work done

Screenshot

To be done

Regression Notes

peril-wordpress-mobile bot commented Sep 21, 2021 • edited

peril-wordpress-mobile bot commented Sep 21, 2021 • edited

peril-wordpress-mobile bot commented Sep 21, 2021

jd-alexander left a comment

Choose a reason for hiding this comment

jd-alexander left a comment

Choose a reason for hiding this comment

jd-alexander commented Sep 21, 2021

dnalves commented Sep 21, 2021

jd-alexander commented Sep 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jd-alexander commented Sep 23, 2021 • edited

jd-alexander commented Sep 23, 2021

Choose a reason for hiding this comment

dnalves Sep 24, 2021 • edited

Choose a reason for hiding this comment

dnalves commented Sep 24, 2021

jd-alexander commented Sep 24, 2021 • edited

dnalves commented Sep 24, 2021

jd-alexander left a comment

Choose a reason for hiding this comment

dnalves commented Sep 18, 2021 •

edited

peril-wordpress-mobile bot commented Sep 21, 2021 •

edited

peril-wordpress-mobile bot commented Sep 21, 2021 •

edited

jd-alexander commented Sep 23, 2021 •

edited

dnalves Sep 24, 2021 •

edited

jd-alexander commented Sep 24, 2021 •

edited