Introduce new EventBus adapter #4130

waiting-for-dev · 2021-07-13T09:51:55Z

Description

We're deprecating Spree::Event::Adapters::ActiveSupportNotifications
for a couple of reasons:

It uses the same bus that standard Rails notifications. It forces us
to try to normalize the event names given through Spree::Event.fire
& Spree::Event.subscribe to add a .spree suffix. However, it's not
a complete solution as we're missing subscriptions placed through a
regular expression (like Spree::Event.subscribe /.*/, which
subscribes to all events, including Rails ones).
It relies heavily on global state. Besides not being able to create
two different buses (see the previous point), it makes it cumbersome
to test events in isolation.

This commit introduces a new Spree::Event::Adapters::EventBus adapter,
which instances make for independent event buses. As our implementation
of the event bus system relies on a global bus for all Solidus events,
the idea is to initialize a single instance and configure it as
Spree::Config.events.adapter. However, we've added a new optional
adapter: parameter to all relevant methods in Spree::Event to inject
other buses when testing. E.g.:

bus = Spree::Event::Adapters::EventBus.new
Spree::Event.subscribe('foo', adapter: bus) { do_something }
Spree::Event.fire('foo', adapter: bus)

The old adapter is still the default one due to backward compatibility
reasons. However, we display a deprecation warning to let users know
that they're encouraged to change. There're two critical updates that
they need to be aware of (they're also described in the deprecated
message):

Event name normalization won't happen anymore. E.g. in order to
subscribe to all Solidus events probably users will need to change
from Spree::Event.subscribe /\.spree$/ to Spree::Event.subscribe /.*/. There's no way to reliably warn when the .spree prefix is
used, as users can still decide to use it as far as they're consistent
when performing different operations. On top of that, we still can't
inspect regular expressions with confidence.
The old adapter allowed to provide a block on Spree::Event.fire, like in:
```
Spree::Event.fire 'foo', order: order do
  order.do_something
end
```
As this block is entirely unnecessary and it can lead to
confusion (is it executed before or after the subscribers?), it's no
longer supported in the new adapter. To provide a safer upgrade
experience, we raise an ArgumentError when the new adapter is being
used, and a block is provided.

The new EventBus adapter will be made the default one on the next
major Solidus release.

The old adapter is leaking the abstraction through the returned values
(ActiveSupport::Notifications::Fanout::Subscribers::Timed instances).
In the new adapter, we introduce Spree::Event::Event &
Spree::Event::Listener objects that the adapters need to return.

The main Spree::Event module will still keep a copy of the
subscriptions when the legacy adapter is used. This behavior was
introduced to differentiate Solidus events from others, but it can cause
inconsistencies if we compare it with the copy on the adapter. As
something no longer needed, we're branching to don't perform the logic
when the adapter is the new one.

Checklist:

I have followed Pull Request guidelines
I have added a detailed description into each commit message
I have updated Guides and README accordingly to this change (if needed)
I have added tests to cover this change (if needed)
I have attached screenshots to this PR for visual changes (if needed)

core/lib/spree/event.rb

core/lib/spree/event/listener.rb

core/spec/lib/spree/event_spec.rb

core/lib/spree/event.rb

kennyadsl

Made a first pass and left some questions, great work as usual.

kennyadsl · 2021-07-13T11:17:22Z

core/lib/spree/event.rb

    #
    # @example Trigger an event named 'order_finalized'
    #   Spree::Event.fire 'order_finalized', order: @order do
    #     @order.finalize!
    #   end
+    #
+    # TODO: Change opts to be keyword arguments and include `adapter:` in them


Is this TODO for this PR?

I'd say no, but let me explain it better.

So, the issue is that I'd like to be consistent with the other methods, as I do think that having adapter: as a keyword argument is the most expressive way to inject adapters. However, if I add it to the signature:

def fire(event_name, opts = {}, adapter: default_adapter)

There's no way for it to work on Ruby < 3 unless we provide the adapter: argument when invoking it (which breaks the whole purpose). I.e., both

fire('foo', order: order)

and

fire('foo', { order: order })

will raise ArgumentError (unknown keyword: :order).

Given that, we should move opts from being a Hash to keyword arguments:

def fire(event_name, adapter: default_adapter, **kwargs)

It will work in RUBY < 3, both with:

fire('foo', order: order)

and

fire('foo', { order: order })

Still, Ruby 3 stores are now expecting that the event options are given as a Hash, so it will break them. In addition, we can't provide a straightforward deprecation path as if we try to support both options:

def fire(event_name, opts = {}, adapter: default_adapter, **kwargs)

we're back to square one.

I can think of two workarounds for it:

Define different methods depending on RUBY_VERSION. E.g. (pseudo-code):

if RUBY_VERSION < 3 def fire(event_name, adapter: default_adapter, **kwargs) # ... else def fire(event_name, opts = {}, adapter: default_adapter, **kwargs) opts = if opts.any? deprecate('Use kwargs instead of hash') opts else kwargs end

I think this option would deserve a separate commit.

Just change the signature to:

def fire(event_name, adapter: default_adapter, **kwargs)

on Solidus 4 and inform about the change on the upgrade guide.

Got it. What if we use opts for now and deprecate all the un-recognized options passed? So that when we'll switch to kwargs in Solidus 4 we'll sure to be compliant.

# Pseudo-code def fire(event_name, opts = {}, adapter = opts.delete :adapter order = opts.delete :order opts.each do |key, value| Spree::Deprecation.warn("passing #{key} to fire is deprecated, please just pass order and adapter") end end

The problem is that when you fire an event, you can attach to it whatever information you want to make available to the subscribers. Here I used order, but it could be anything else 🙂

Oh right, how did I miss it? I see no way to deprecate it before removal then, maybe just changing the method name and deprecate fire at all but I don't think it's worth it. We can specify in the TODO that we can remove it in the next major, adding the keyword Deprecation somehow so that we find it along with the others when removing 3.x deprecations.

Alternatively we could create a new issue for this, and move it into a new 4.0 milestone in GitHub.

I added the improved TODO: comment with Spree::Deprecation on it a link to this comment 🙂

kennyadsl · 2021-07-13T11:19:15Z

core/lib/spree/event.rb

+
+        MSG
+      elsif block_given?
+        yield opts


I think we need to emit a deprecation warning here, otherwise people using AS::N adapter won't see that they need to remove the block in their fire calls. They will only know this with an exception after switching to the new adapter when fire will be called.

The main deprecation message, the one they should see when booting up the app or running the test suite, is already informing users to stop providing blocks to Spree::Event.fire. Do you think it's not enough? In fact, the block is still supported if they're using the legacy adapter.

I saw that the main deprecation warning mentions that people need to stop using fire with block, but that code could technically be in some extension and they won't be able to know it until the code breaks. Honestly, I don't think it's super used in the real world but I'd still add a deprecation warning here, just to be 100% sure.

I see. Added!

kennyadsl · 2021-07-13T11:21:38Z

core/lib/spree/event.rb

-    def unsubscribe(subscriber)
-      name_or_subscriber = subscriber.is_a?(String) ? normalize_name(subscriber) : subscriber
+    def unsubscribe(name_or_subscriber, adapter: default_adapter)
+      name_or_subscriber = normalize_name(name_or_subscriber)


I don't get how normalize_name is handling the case when name_or_subscriber is a subscriber. Will that just be a symbol in that case? In the method documentation I see also Spree::Event::Listener

normalize_name is Spree::Event is just returning the argument untouched unless it's a Symbol. So, if it's a Spree::Event::Listener it won't do anything. I agree it'd be clearer to ask whether it's a Listener before dispatching it to normalize_name, but as we can be dealing with an ActiveSupport...Fanout object from the legacy adapter it'd make the code rather ugly. WDYT?

My only concern is that looking at the normalize_name method as is, I assume that name can be a String or a Symbol, even if as you pointed out, it can be any object. Not sure if any action is needed honestly, the code looks good, maybe it's just me. :)

I think you're right. We can't let the legacy adapter condition a better design. I refactored the method so that normalize_name is only called when it's not a subscriber object from any of the adapters.

core/lib/spree/event.rb

core/lib/spree/event/configuration.rb

kennyadsl · 2021-07-13T11:34:59Z

core/lib/spree/event/configuration.rb

+      private
+
+      def deprecate_if_legacy_adapter(adapter)
+        Spree::Deprecation.warn <<~MSG if adapter == Spree::Event::Adapters::ActiveSupportNotifications && !ENV['CI']


Why we need !ENV['CI']?

The current version of Solidus is still using the legacy adapter, even though it's warning about its deprecation. CircleCI is currently configured to raise when any warning is displayed:

solidus/.circleci/config.yml

Line 11 in 7118543

SOLIDUS_RAISE_DEPRECATIONS: true

The CI environment variable is set on CircleCI, making it possible to have a green build.

However, the warnings are indeed displayed when running bin/rspec locally. We could create a custom ENV variable in the spec_helper files and check it instead of CI, but I'm not a fan of this solution as it's too easy for that variable definitions to say there once they're no longer needed. Remember that we can't just ignore the warning when RAILS_ENV is test as we do want it to be displayed on users' application test suite. Do you think we have a better alternative?

Do you think we have a better alternative?

I think we could set up the dummy app to test against the new adapter. This means that the deprecated code won't be tested by default, which is not ideal because people may still use it. We could build a CI matrix (or at least one job) that tests Solidus with the legacy adapter, or we can just create some smoke tests for it, changing the adapter runtime before running those specs. Does it make sense?

Yeah, I think that's the best option we have. I updated the code so that we're using the new adapter on the dummy application. As for the smoke tests, we can use the same that we had before, those on event_spec.rb that reach the legacy adapter.

core/spec/lib/spree/event_spec.rb

aldesantis

@waiting-for-dev thanks, this is amazing! 😍 I have a few change requests, but it's mostly minor stuff. I also have a couple of comments about the general architecture, but I don't know if these can be realistically implemented in a backwards-compatible way.

I'm not fully convinced about Spree::Event being a module, and users having to pass the adapter to each method call if they want to use a non-default adapter. It seems like it would be much cleaner to have a class that you can initialize with the adapter.

When you think about it, Spree::Event is the actual event bus, because it's responsible for coordinating everything. So we could have the following API:

event_bus = Spree::Events::EventBus.new(adapter: custom_adapter)
event_bus.fire ...

Then, in their Solidus initializer, users could just do the following:

Spree.config do |config|
  config.event_bus = Spree::Events::EventBus.new(adapter: Spree::Event::Adapters::Default)
end

As I said, I realize this is a huge departure from the current API, so if you think it's too complicated/doesn't make sense, feel free to let me know.

core/lib/spree/event.rb

core/lib/spree/event/adapters/event_bus.rb

core/lib/spree/event/configuration.rb

core/lib/spree/event/listener.rb

core/lib/spree/event/adapters/event_bus.rb

core/lib/spree/event_bus.rb

core/lib/spree/event_bus/listener.rb

waiting-for-dev · 2021-07-20T08:58:37Z

Spree.config do |config|
  config.event_bus = Spree::Events::EventBus.new(adapter: Spree::Event::Adapters::Default)
end

That would require assigning to Spree::Event, like in Spree::Event = config.event_bus. But, as we feared, the problem is keeping backward compatibility. If Spree::Event is an instance, then we can't define constants under its "namespace". However, Spree::Event::Subscriber is part of the public API. I.e.:

A = Hash.new
class/module A
  class B
  end
end
# => TypeError (A is not a class/module)

waiting-for-dev · 2021-07-20T09:07:16Z

I added another commit making Spree::Event to return the Spree::Event::Event object instead of the list of listeners.

aldesantis

@waiting-for-dev thanks, looks great! <3

We're deprecating `Spree::Event::Adapters::ActiveSupportNotifications` for a couple of reasons: - It uses the same bus that standard Rails notifications. It forces us to try to normalize the event names given through `Spree::Event.fire` & `Spree::Event.subscribe` to add a `.spree` suffix. However, it's not a complete solution as we're missing subscriptions placed through a regular expression (like `Spree::Event.subscribe /.*/`, which subscribes to all events, including Rails ones). - It relies heavily on global state. Besides not being able to create two different buses (see the previous point), it makes it cumbersome to test events in isolation. This commit introduces a new `Spree::Event::Adapters::Default` adapter, which instances make for independent event buses. As our implementation of the event bus system relies on a global bus for all Solidus events, the idea is to initialize a single instance and configure it as `Spree::Config.events.adapter`. However, we've added a new optional `adapter:` parameter to all relevant methods in `Spree::Event` to inject other buses when testing. E.g.: ```ruby bus = Spree::Event::Adapters::Default.new Spree::Event.subscribe('foo', adapter: bus) { do_something } Spree::Event.fire('foo', adapter: bus) ``` The old adapter is still the default one due to backward compatibility reasons. However, we display a deprecation warning to let users know that they're encouraged to change. There're two critical updates that they need to be aware of (they're also described in the deprecated message): - Event name normalization won't happen anymore. E.g. in order to subscribe to all Solidus events probably users will need to change from `Spree::Event.subscribe /\.spree$/` to `Spree::Event.subscribe /.*/`. There's no way to reliably warn when the `.spree` prefix is used, as users can still decide to use it as far as they're consistent when performing different operations. On top of that, we still can't inspect regular expressions with confidence. - The old adapter allowed to provide a block on `Spree::Event.fire`, like in: ```ruby Spree::Event.fire 'foo', order: order do order.do_something end ``` As this block is entirely unnecessary and it can lead to confusion (is it executed before or after the subscribers?), it's no longer supported in the new adapter. To provide a safer upgrade experience, we raise an `ArgumentError` when the new adapter is being used, and a block is provided. We also show a warning when provided on the legacy adapter. The new `Default` adapter will be made the default one on the next major Solidus release. The old adapter is leaking the abstraction through the returned values (`ActiveSupport::Notifications::Fanout::Subscribers::Timed` instances). In the new adapter, we introduce `Spree::Event::Event` & `Spree::Event::Listener` objects that the adapters need to return. The main `Spree::Event` module will still keep a copy of the subscriptions when the legacy adapter is used. This behavior was introduced to differentiate Solidus events from others, but it can cause inconsistencies if we compare it with the copy on the adapter. As something no longer needed, we're branching to don't perform the logic when the adapter is the new one. We're also adapting the dummy app used in testing to use the future default adapter not to see the deprecation messages. However, the old tests that went through the legacy adapter are still in place.

spaghetticode

@waiting-for-dev definitely amazing work here! 🤩

I'm sorry it took so long to review the PR, I started a couple of times before but the changes were harder to digest than I anticipated.

I found reviewing the PR a little more difficult than usual, partly because of the PR subject, then I think because it's basically one single commit.
I tend to prefer when big PRs are spread among multiple commits, as they help in scoping code changes and their explanations. For example, I spent some amount of time trying to understand if the new implementation of Spree::Event.normalize_name could possibly lead to bugs (it was previously delegated to the adapter and the new definition is surely different), so I needed to check in the whole PR what areas were affected by this change, eventually realizing that the AS Notification adapter has been properly updated as well in order to still use the old definition. I think that, if this change was scoped in a single commit, this would have been easier to verify.

spaghetticode · 2021-07-28T14:46:47Z

core/lib/spree/event.rb

    #
-    # @see Spree::AppConfiguration
-    def adapter


should we deprecate this method before removing it?

Hmmm, do we consider it a public method? Indeed, it's not marked as @private, so it's effectively a public method. But in which situation would a user be calling Spree::Event.adapter when we had a single adapter up to this point? I'm not against it, but I'm just wondering if it's necessary. WDYT? If you think it's better to add it, I will.

Given this is a public API, and that this is a prominent Solidus feature, I would deprecate the method first, unless this creates any issues in our path forward. It doesn't require much work, and it keeps us on the safe side.

Makes sense. Done!

spaghetticode · 2021-08-04T15:37:54Z

core/lib/spree/testing_support/dummy_app.rb

@@ -133,6 +133,9 @@ class Application < ::Rails::Application
 Spree.user_class = 'Spree::LegacyUser'
 Spree.config do |config|
  config.mails_from = "[email protected]"
+  # TODO: Remove on Solidus 4.0 as it'll be the default
+  require 'spree/event/adapters/default'
+  config.events.adapter = Spree::Event::Adapters::Default.new


I'm not 100% convinced by this. If the old adapter is still the default one used by Solidus, I think we should run the suite against it. I read in a previous discussion that the issue is mostly deprecation warnings on CI: I think there could be a couple of solutions to this issue:

don't deprecate the old behavior right now, but leave that to a subsequent PR (that would be a short lived victory, but it would also move some code out of this PR, making it smaller);

I think we could silence the deprecations with something like allow(Spree::Deprecation).to receive(:warn) so they don't make CI fail

Yeah, I'm neither 100% convinced. Ideally, we'd run two jobs, each one with a different adapter, but as we're limited here, we have to decide. I'd go with 2 or with the previous solution (adding a CI environment variable check) if we prefer to check the legacy adapter. However, the former would clutter our test suite. Thoughts @kennyadsl ?

Another solution, but don't know how feasible would be, is to have the event bus specs as shared examples and run them with both adapters. I think this could be handy also in the future, if we ever decide to change the default adapter again, or if we keep multiple official ones in the codebase.

I think that the main problem is not the specs on the event bus itself but those at the integration level. We already have tests for both adapters, and I don't think it's worth trying to extract them to a shared example. The reason is that both adapters don't support the exact number of features. This is more evident in the work that will be built on top of this PR. They're features that are difficult or impossible to build with ASN. On top of that, I already talked with @aldesantis about this concept of adapters for the event bus. Again, the work that will follow this PR adds more complexity and therefore becomes more opinionated, probably making it more difficult or impossible to build good abstractions for other libraries. In my view, we should keep the adapter pattern only until the old adapter is completely deprecated and removed, and from that point on, I'd remove the adapters concept altogether.

Ideally, we should use two CI jobs, one for each adapter. If that's not possible, we should decide in favor of one or the other. I have just been playing with options to provide a rspec helper to run tests for both adapters, but it's tricky, and there is no way to ensure they are used if we add new tests. If we use them at the example level, the adapter will be ignored on any before, let... blocks, where a Spree::Event.subscribe could happen. And using them at the RSpec's class level (wrapping context, before... blocks) doesn't work as expected, as the examples are declared at the class level (when the RSpec file is loaded) but run at the instance level (once the adapter has been changed again to be the default one).

Oh, I didn't know we are planning to remove the concept of the adapter from Spree::Event... then it definitely doesn't make any sense to build a multi-adapter testing facility.

I guess that from a pragmatic point of view it would make sense to test the new adapter since it will become the new default and we don't plan to change anything on the old one, but on the other hand, since the old one is still the default, we need to make sure it keeps working properly as we introduce new features in order to avoid breaking Solidus installations... sounds like a tough call.

Oh, I didn't know we are planning to remove the concept of the adapter from Spree::Event

Well, it's not something 100% decided. It's more that, with the new additions to the event bus, I think that the adapters concept is ceasing to be meaningful, and I talked about it with @aldesantis. But I don't think the decision has been made, so other views are more than welcome 🙂

I'm also not 100% convinced here. Considering that we are doing to merge other changes in the Events context, I think it's very likely that we could have some effect on the legacy adapter and if we always default the test suite to the new adapter we have really no way to notice that. Even if deprecated, we should still be sure it's working until it's removed. Even in another PR, I'd experiment with adding another CI job for this, maybe removing another existing job (paperclip, which should be removed/unsupported now if I recall correctly).

I thought that adding a new CI job was off the table, but that's definitely the best solution. I didn't see a job for paperclip, so probably it's been already removed.

Please, take a look at #4176

waiting-for-dev · 2021-08-05T07:15:29Z

@waiting-for-dev definitely amazing work here!

I'm sorry it took so long to review the PR, I started a couple of times before, but the changes were harder to digest than I anticipated.

I found reviewing the PR a little more difficult than usual, partly because of the PR subject, then I think because it's basically one single commit.
I tend to prefer when big PRs are spread among multiple commits, as they help in scoping code changes and their explanations. For example, I spent some amount of time trying to understand if the new implementation of Spree::Event.normalize_name could possibly lead to bugs (it was previously delegated to the adapter and the new definition is surely different), so I needed to check in the whole PR what areas were affected by this change, eventually realizing that the AS Notification adapter has been properly updated as well in order to still use the old definition. I think that, if this change was scoped in a single commit, this would have been easier to verify.

Thanks for your review, @spaghetticode! I addressed your comments. Sorry for the difficulties in reviewing it. The truth is that the path to take wasn't clear at the beginning of this work, so the first phase was a good amount of exploratory coding. Fortunately, the rest of the work on top of this commit is well structured in different PRs.

kennyadsl · 2021-08-09T10:25:34Z

Let's hold this for a while. We'll release Solidus 3.1 first so that all the new events-related changes coming will go in a single release.

spaghetticode

Although I'm not entirely convinced here I'm approving the PR since I believe it's more important to move this feature forward.... thanks @waiting-for-dev!

kennyadsl

Approved, but left a comment that I think we should address soon. In the meantime, we can merge this, thanks a lot Marc!

This commit adds a new job on CI to perform the test suite with the legacy adapter configured. Besides that, it extracts the deprecation logic into its module to avoid having it scattered through different parts of the system. Relates to discussion: solidusio#4130 (comment)

houndci-bot reviewed Jul 13, 2021

View reviewed changes

core/lib/spree/event.rb Outdated Show resolved Hide resolved

core/lib/spree/event.rb Outdated Show resolved Hide resolved

core/lib/spree/event/listener.rb Show resolved Hide resolved

core/spec/lib/spree/event_spec.rb Outdated Show resolved Hide resolved

waiting-for-dev force-pushed the waiting-for-dev/events branch 3 times, most recently from b59a236 to 817a9ce Compare July 13, 2021 10:12

houndci-bot reviewed Jul 13, 2021

View reviewed changes

core/lib/spree/event.rb Outdated Show resolved Hide resolved

waiting-for-dev force-pushed the waiting-for-dev/events branch from 817a9ce to 5c3966d Compare July 13, 2021 10:13

kennyadsl reviewed Jul 13, 2021

View reviewed changes

houndci-bot reviewed Jul 15, 2021

View reviewed changes

core/spec/lib/spree/event_spec.rb Outdated Show resolved Hide resolved

waiting-for-dev force-pushed the waiting-for-dev/events branch from 3c50465 to 5bcdc8c Compare July 15, 2021 07:25

aldesantis requested changes Jul 16, 2021

View reviewed changes

houndci-bot reviewed Jul 20, 2021

View reviewed changes

core/lib/spree/event_bus.rb Outdated Show resolved Hide resolved

core/lib/spree/event_bus/listener.rb Outdated Show resolved Hide resolved

waiting-for-dev force-pushed the waiting-for-dev/events branch from 81428ca to a1913a8 Compare July 20, 2021 08:53

aldesantis approved these changes Jul 20, 2021

View reviewed changes

waiting-for-dev force-pushed the waiting-for-dev/events branch from 18f66f3 to 17198b3 Compare July 23, 2021 03:05

waiting-for-dev force-pushed the waiting-for-dev/events branch from 17198b3 to e1662c2 Compare July 29, 2021 09:33

aldesantis requested a review from a team July 29, 2021 10:39

aldesantis added the Needs Core Team Review label Jul 29, 2021

Be clear with expectations

fe3ca1a

spaghetticode reviewed Aug 4, 2021

View reviewed changes

kennyadsl added the release:major Breaking change on hold until next major release label Aug 9, 2021

Deprecate Spree::Event.adapter

c22270b

kennyadsl modified the milestones: 3.1.0, 3.2.0 Sep 8, 2021

jarednorman approved these changes Sep 15, 2021

View reviewed changes

spaghetticode approved these changes Sep 17, 2021

View reviewed changes

kennyadsl approved these changes Sep 22, 2021

View reviewed changes

kennyadsl merged commit efeba4e into solidusio:master Sep 22, 2021

waiting-for-dev removed the release:major Breaking change on hold until next major release label Sep 23, 2021

waiting-for-dev mentioned this pull request Sep 23, 2021

Centralize legacy event bus deprecation and test legacy on CI #4176

Merged

5 tasks

waiting-for-dev mentioned this pull request Apr 15, 2022

Use Omnes for pub/sub #4342

Merged

5 tasks

waiting-for-dev deleted the waiting-for-dev/events branch September 4, 2023 09:49

Introduce new EventBus adapter #4130

Introduce new EventBus adapter #4130

Conversation

waiting-for-dev commented Jul 13, 2021

kennyadsl left a 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

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

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

aldesantis left a comment

Choose a reason for hiding this comment

waiting-for-dev commented Jul 20, 2021 • edited Loading

waiting-for-dev commented Jul 20, 2021

aldesantis left a comment

Choose a reason for hiding this comment

spaghetticode left a 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

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

waiting-for-dev commented Aug 5, 2021

kennyadsl commented Aug 9, 2021

spaghetticode left a comment

Choose a reason for hiding this comment

kennyadsl left a comment

Choose a reason for hiding this comment

waiting-for-dev commented Jul 20, 2021 •

edited

Loading