📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.

	App Name	WooCommerce Android
	Platform	📱 Mobile
	Flavor	Jalapeno
	Build Type	Debug
	Commit	b30300d
	Direct Download	woocommerce-prototype-build-pr12655-b30300d.apk

@kidinov Converted this to draft as I realised that we need to maintain notification order for displaying it in group notifications and ConcurrentHashMap doesn't maintain insertion order. I need to think more about the solution and the risk of causing regression before coming up with the solution. I'm putting this PR on hold for now.

@AnirudhBhat 👋

I think this may work:
val map: MutableMap<String, Int> = Collections.synchronizedMap(LinkedHashMap())

In this case, we likely need to add a Synchronized block wherever we iterate over the map, which would be similar to our current solution but more complex and harder to read. What do you think?

@AnirudhBhat 👋

In this case, we likely need to add a Synchronized block wherever we iterate over the map

Not sure I am following. Why so?

Not sure I am following. Why so?

AFAIU, SynchronizedMap requires manual synchronisation when iterating over its map. The documentation says this as well.

I could confirm this by changing the notifications map to Collections.synchronizedMap(LinkedHashMap()) and running the unit test would fail.

Let me know if I'm missing something. Thanks!

AFAIU, SynchronizedMap requires manual synchronisation when iterating over its map. The documentation says this as well.

My understanding of the documentation is that you need to sync on the map when you are iterating over anything that returned from it, e.g iterator, keys etc

Having said that, I looked up the code, and I don't understand where we need an insertion order. Could you please point me out?

Also, do you understand why asSequence put everywhere?

Having said that, I looked up the code, and I don't understand where we need an insertion order. Could you please point me out?

1. We use mutableMap which internally uses LinkedHashMap that maintains insertion order
2. There are few tests that verify insertion order along with others
   when two new order notifications are received for different stores display the notification correctly
when two new review notifications are received for the same store, then display the notification correctly
when two new order notifications are received for the same store, then display the notification correctly

and few more

Also, do you understand why asSequence put everywhere?

I'm not sure. Maybe for performance gains?

@AnirudhBhat 👋

We use mutableMap which internally uses LinkedHashMap that maintains insertion order
Yep, I got that; the question is if we need to maintain insertion order. I can not find that in the code - seems all reads are by id 🤔

There are few tests that verify insertion order along with others

I tried to change the implementation of the map to ConcurrentHashMap -> when two new order notifications are received for the same store, then display the notification correctly. Spent some time - could not figureout what exactly the test tests though 😀

I'm not sure. Maybe for performance gains?

In most cases, there is no performance gain, though, as we just iterate over the map. In a couple of places we filter so it'll give minimal performance, but we probably talking about a few notifications here so probably negletable

Version 20.6 has now entered code-freeze, so the milestone of this PR has been updated to 20.7.

Version 20.8 has now entered code-freeze, so the milestone of this PR has been updated to 20.9.

Version 20.9 has now entered code-freeze, so the milestone of this PR has been updated to 21.0.

Version 21.0 has now entered code-freeze, so the milestone of this PR has been updated to 21.1.

Version 21.1 has now entered code-freeze, so the milestone of this PR has been updated to 21.2.