Compose area: Enforce mandatory topics #4798

thisisnitish · 2021-06-12T08:28:37Z

Fix: Add the relevant property in Files, which fixes the first bullet point of #4378, which tells that messages must have topics.
Fixes: first bullet point of #4378
Changes:

added realm_mandatory_topics to InitialDataTypes
stored the relevant settings in RealmState
added isTopicMandatory to initialState and set to false
populating the realmReducer

[chris: also added commits to use the setting and fix #4378]

Fixes: #4378

WesleyAC · 2021-06-14T20:27:56Z

This looks good to me, but I'm going to wait for @gnprice or @chrisbobbe to give a second opinion before merging :)

chrisbobbe

Thanks @thisisnitish (and @WesleyAC for the review)!

See one comment below, about a migration.

Please format the commit message according to our style guide. I think the change is probably simple enough that it can be described in just a summary line (with no body text), but the summary line should be formatted properly with a prefix, a period at the end, etc., as described in the doc.

chrisbobbe · 2021-06-14T20:41:08Z

src/realm/realmReducer.js

@@ -24,6 +24,7 @@ const initialState = {
  twentyFourHourTime: false,
  canCreateStreams: true,
  isAdmin: false,
+  isTopicMandatory: false,


Let's add a dropCache migration in src/boot/store.js for this. Something like,

// Add `isTopicMandatory` to `state.realm`. '30': dropCache,

(or a higher number than 30, if new migrations happen to get added after I post this review). See dropCache's jsdoc for what it's for.

Does this actually matter? My understanding is that an app update will drop the cache anyways, since it'll force a restart, and we aren't persisting this data in any case. I could be misunderstanding how long we intend for this data to persist, though.

we aren't persisting this data in any case

Ah: we are persisting this data to disk. realmReducer deals with state.realm, and 'realm' is in cacheKeys. Here's the jsdoc on cacheKeys:

/** * Properties on the global state which we persist for caching's sake. * * These represent information for which the ground truth is on the * server, but which we persist locally so that we have it cached and * don't have to re-download it. */

That data won't get cleared automatically on an app restart, but it's easy and harmless for us to tell it to clear on a version upgrade, by using a dropCache migration.

Hmm, but since this is getting overwritten on the /register response, doesn't that mean that it's going to get overwritten on app load anyways, since we're going to call /register then, before we display anything?

(To be clear, I think it's reasonable to have a dropCache in either case, I'm just trying to make sure I have a good understanding of the data flow)

(To be clear, I think it's reasonable to have a dropCache in either case, I'm just trying to make sure I have a good understanding of the data flow)

Makes sense! 🙂

We do expect this to get clobbered with an up-to-date value when a /register completes. But there are two important possible states to consider while that /register is in progress:

We have no cached server data to show, so we show FullScreenLoading.

We have some stale data for this account, cached from a previous session, so we show an interactable UI with the "Connecting..." banner (LoadingBanner).

The conditional for what to show is provided by useHaveServerDataGate.

(The logic for what to show used to be more complicated and pretty hard to verify, depending on navigating imperatively at the right time, and setting the initial nav state correctly. If you want, you can read through #4454—particularly, a023b75—for where we looked at that logic closely and refactored it by removing 'loading' as a navigation route; we then consolidated a reusable "have-server-data gate" in #4603 for use with components besides MainTabsScreen, and we fixed a bug in that "gate" logic in 98d19b8 just a few weeks ago.)

In that second state—

We have some stale data for this account, cached from a previous session, so we show an interactable UI with the "Connecting..." banner.

—it's possible, in theory (and I guess in practice, if the /register takes super long and we fix various UI perf issues) to go poking around the app and try to do things with the stale data that you'd also want to do with up-to-date data. Like, I guess, try to send a message without a topic. So it's good to do something explicit like this to ensure we have the right data from the beginning.

So it's good to do something explicit like this to ensure we have the right data from the beginning.

Hmm, even more explicit than dropCache would be if the migration were written to add the key explicitly. I think dropCache would work, but only because of the "shallow-merge" quirk in redux-persist I noticed over at #4679 (comment). (And that same quirk may have the effect that things would work even if we omitted a migration altogether—but as I said in that linked thread, it's nice to be as explicit as possible with migrations.)

I ~copied the dropCache suggestion from #4543 (comment).

So, now I'm thinking something like

// Add `isTopicMandatory` to `state.realm`. '30': state => ({ ...state, realm: { ...state.realm, isTopicMandatory: false, }, }),

Sorry for the confusion! And thanks Wesley for causing me to think through this again. 🙂

And that same ["shallow-merge"] quirk may have the effect that things would work even if we omitted a migration altogether

Yes, that's right. Effectively it means that when we introduce a new top-level property to any of our top-level state subtrees -- anything of the form state.foo.bar -- then without doing anything about it in a migration, we get the same effect as if we had a migration to give it the same value that we have for it in the initial state, as specified in the reducer.

A key bit of the background to how it works out that way is that the top-level subtrees state.foo are the units of what we store with redux-persist.

I think dropCache would work, but only because of the "shallow-merge" quirk

The reason dropCache works is similar in spirit but I think it's more basic: it's that if a given top-level subtree is missing entirely, then at rehydrate time we just let the initial state (as specified in the reducer) stand untouched.

So in a situation like this, where we've introduced a new property state.foo.bar on one of the top-level state subtrees state.foo, the result is that any of:

no migration

a dropCache migration

a migration that explicitly adds the new property, like in Chris's last comment above

will produce the same, correct results.

It's good to be explicit, as with option 3. On the other hand, it means we duplicate the detail of what the default is -- new installs will get the default from the reducer's initial state, while old installs upgrading will get it from the migration. It also means more boilerplate, in a spot that isn't really type-checked (and is fundamentally tricky to properly type-check) and where getting it wrong could be pretty unpleasant.

So I'm kind of inclined to actually make our practice for this situation be option 1: no migration at all.

However! If we do that, I think it's important to still be explicit about making that choice. That helps us be in the habit of thinking about migrations when updating our types, so that we explicitly think through whether the update is one that falls into this situation and doesn't need a migration. So that could look like adding a comment here, the same place we'd add a migration, that looks like:

// Add `isTopicMandatory` to `state.realm`. // No migration; just use default from initial state.

Scanning back through previous migrations, I think the only one to date that we'd actually have skipped under this approach is 28, adding state.settings.browser. All the others either do something more complicated than adding a property, or add a property somewhere deeper in the tree (like in each element of state.accounts.)

As we add more things like miscellaneous settings, though, this could be something that becomes more common.

Made a chat thread to help make this discussion easier to find: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/data.20migrations/near/1230029

gnprice

Thanks @thisisnitish! Comments below, in addition to Chris's above.

gnprice · 2021-06-16T01:04:16Z

src/api/initialDataTypes.js

@@ -120,6 +120,7 @@ export type RawInitialDataRealmUser = {|
  is_admin: boolean,
  realm_users: Array<{| ...User, avatar_url?: string | null |}>,
  realm_non_active_users: Array<{| ...User, avatar_url?: string | null |}>,
+  realm_mandatory_topics: boolean,


We already have this on InitialDataRealm, so no need to add it elsewhere.

The right place for it is indeed InitialDataRealm. The organization of these different types is described at the place where they're all combined together:

// Initial data snapshot sent in response to a `/register` request, // after validation and transformation. export type InitialData = {| // The server sends different subsets of the full available data, // depending on what event types the client subscribes to with the // `fetch_event_types` field of the `/register` request. We name these // subsets after the event types that cause them to be included.

This field is controlled by the realm event type being present; from the docs at https://zulip.com/api/register-queue :

realm_mandatory_topics: boolean Present if realm is present in fetch_event_types.

so InitialDataRealm is the right place.

It'd belong on InitialDataRealmUser (and therefore here on RawInitialDataRealmUser, which that's based on) if it were instead controlled by realm_user.

(This organization isn't totally obvious, even though there is that one comment about it. I may go and add some short jsdoc on the individual types here.)

gnprice · 2021-06-16T01:07:41Z

src/reduxTypes.js

@@ -256,6 +256,7 @@ export type RealmState = {|
  twentyFourHourTime: boolean,
  canCreateStreams: boolean,
  isAdmin: boolean,
+  isTopicMandatory: boolean,


Let's keep this name very similar to the name in the API -- that helps simplify understanding what the code is doing, without the reader having to trace through all the layers.

The API's name for this is realm_mandatory_topics, and we're already on the "realm" state. So mandatoryTopics would be a good name.

gnprice · 2021-06-16T01:11:35Z

src/reduxTypes.js

@@ -256,6 +256,7 @@ export type RealmState = {|
  twentyFourHourTime: boolean,
  canCreateStreams: boolean,
  isAdmin: boolean,
+  isTopicMandatory: boolean,


See the note in the jsdoc on this RealmState type about how the properties are organized: stuff about the server, then about the org/realm, then about the user.

This property is about the org aka the realm -- that's why its name in the API is prefixed realm_ and it's controlled by the realm event type -- so it belongs in the "realm" section.

(I realize this is kind of easy to miss. I may try moving that jsdoc material inline into the type definition, next to the respective properties.)

gnprice · 2021-06-16T01:30:26Z

src/realm/realmReducer.js

@@ -24,6 +24,7 @@ const initialState = {
  twentyFourHourTime: false,
  canCreateStreams: true,
  isAdmin: false,
+  isTopicMandatory: false,


And that same ["shallow-merge"] quirk may have the effect that things would work even if we omitted a migration altogether

Yes, that's right. Effectively it means that when we introduce a new top-level property to any of our top-level state subtrees -- anything of the form state.foo.bar -- then without doing anything about it in a migration, we get the same effect as if we had a migration to give it the same value that we have for it in the initial state, as specified in the reducer.

A key bit of the background to how it works out that way is that the top-level subtrees state.foo are the units of what we store with redux-persist.

I think dropCache would work, but only because of the "shallow-merge" quirk

The reason dropCache works is similar in spirit but I think it's more basic: it's that if a given top-level subtree is missing entirely, then at rehydrate time we just let the initial state (as specified in the reducer) stand untouched.

So in a situation like this, where we've introduced a new property state.foo.bar on one of the top-level state subtrees state.foo, the result is that any of:

no migration

a dropCache migration

a migration that explicitly adds the new property, like in Chris's last comment above

will produce the same, correct results.

It's good to be explicit, as with option 3. On the other hand, it means we duplicate the detail of what the default is -- new installs will get the default from the reducer's initial state, while old installs upgrading will get it from the migration. It also means more boilerplate, in a spot that isn't really type-checked (and is fundamentally tricky to properly type-check) and where getting it wrong could be pretty unpleasant.

So I'm kind of inclined to actually make our practice for this situation be option 1: no migration at all.

However! If we do that, I think it's important to still be explicit about making that choice. That helps us be in the habit of thinking about migrations when updating our types, so that we explicitly think through whether the update is one that falls into this situation and doesn't need a migration. So that could look like adding a comment here, the same place we'd add a migration, that looks like:

// Add `isTopicMandatory` to `state.realm`. // No migration; just use default from initial state.

Scanning back through previous migrations, I think the only one to date that we'd actually have skipped under this approach is 28, adding state.settings.browser. All the others either do something more complicated than adding a property, or add a property somewhere deeper in the tree (like in each element of state.accounts.)

As we add more things like miscellaneous settings, though, this could be something that becomes more common.

thisisnitish · 2021-06-16T18:07:09Z

Hey @chrisbobbe and @WesleyAC , I've added a summary, Please have a look and let me know what I can do more 🙂✌️.

chrisbobbe · 2021-06-16T18:15:38Z

Hey @chrisbobbe and @WesleyAC , I've added a summary, Please have a look and let me know what I can do more 🙂✌️.

I think you haven't force-pushed your new revision yet. 🙂 Also, I see that you've changed the title of this PR. Have you added more commits that make the new title an accurate description?

Also, please address Greg's review, too, if you haven't yet.

thisisnitish · 2021-06-16T18:35:15Z

Hey @chrisbobbe and @WesleyAC , I've added a summary, Please have a look and let me know what I can do more 🙂✌️.

I think you haven't force-pushed your new revision yet. 🙂 Also, I see that you've changed the title of this PR. Have you added more commits that make the new title an accurate description?

Also, please address Greg's review, too, if you haven't yet.

Hey @chrisbobbe, I've force pushed the commit. Yes, I've changed the title of the PR so that it looks more descriptive, and I've added the suitable commit message. Please have a look and Yes I've to address the Greg's review, I'll do it 🙂👍🏻.

chrisbobbe · 2021-06-16T18:46:50Z

Fix: Setup the 'real_mandatory_topics' as 'isTopicMandatory' in Redux by adding relevant property

The prefix should be "the name of the subsystem and a colon", and it looks like that summary line is 97 characters long. Please take another look at the commit-message style guidelines, linked above. Also, if it's helpful, you can read the history of the mobile project to see example commit messages.

Also, it looks like the PR is still only setting up some data plumbing. This is totally fine; breaking larger work into smaller chunks is generally a good strategy. 🙂 But the PR's new title, "Confirms that messages have topics, If there is "no topic" don't allow", isn't accurate, because that user-facing behavior isn't implemented in the PR.

thisisnitish · 2021-06-21T17:13:46Z

Sorry @chrisbobbe, @WesleyAC and @gnprice for being late, actually I was busy in some personal things, now getting back on track 😊.
Also, I will edit the commit message and PR title and I will also address the Greg's review as soon as possible😊

thisisnitish · 2021-07-01T18:35:47Z

Hey @chrisbobbe sorry for being late again, now I have edited the commit message again and also the PR title and I think the commit is also short. Please have a look and btw I'm addressing the greg review right now 🙂

thisisnitish · 2021-07-03T18:21:19Z

Hey @chrisbobbe, I've read the @gnprice review and at the end as he mentioned to change property name from isTopicMandatory to mandatoryTopics , I've done it 😊. Please have a look 😊.

chrisbobbe · 2021-09-14T21:01:05Z

Thanks, @thisisnitish! I've made a few small tweaks to your commit, and also added some more commits that

go ahead and use the settings value, now that you've gotten it to be stored in Redux (so I'm updating the PR description to say Fixes: #4378), and
do some independent cleanups (one of these is Greg's, cherry-picked from Add stream_id to Outbox values #5000)

Please take a look! Also, @gnprice or @WesleyAC, this should be ready for review

gnprice

Thanks @chrisbobbe for the revised and extended version of this! Looks good, with small comments below.

gnprice · 2021-09-15T01:30:15Z

src/reduxTypes.js

@@ -266,6 +266,7 @@ export type RealmState = $ReadOnly<{|
  twentyFourHourTime: boolean,
  canCreateStreams: boolean,
  isAdmin: boolean,
+  mandatoryTopics: boolean,


nit: this goes in the previous group in this definition:
#4798 (comment)

gnprice · 2021-09-15T01:34:56Z

src/config.js

+  // match
+  //   https://github.com/zulip/zulip/blob/5.0-dev/zerver/lib/actions.py#L2714
+  // or similar logic at the latest `main`.
+  noTopicTopic: '(no topic)',


Centralizing a single definition for this constant makes a lot of sense!

I don't love "config" as a home for it. It's not really so much about a choice of how to configure the app, as it is about something we expect from the server.

Hmm, I guess in fact that suggests a good place for it would be under src/api/ somewhere -- that's where we generally put facts about how the Zulip server behaves. Usually those are in the form of code to invoke, or type definitions, but a constant like this would fit too.

I think a file like src/api/constants.js, or src/api/apiConstants.js, would work well.

Oh, and zulip/zulip#3639 is the issue for the fact that this is hardcoded in the server (and therefore untranslated.)

chrisbobbe · 2021-10-18T21:03:18Z

Thanks for the review, @gnprice! Revision pushed.

gnprice

Thanks @chrisbobbe! A few comments below, all I think easy to resolve.

(Well, one points at a hairier issue, but I think a TODO comment is a fine solution for this PR.)

gnprice · 2021-10-18T21:33:01Z

src/api/constants.js

+ */
+// This is hardcoded in the server, and therefore untranslated; that's
+// zulip/zulip#3639.
+export const NO_TOPIC_TOPIC: '(no topic)' = '(no topic)';


I think this is one where the type is best written as string rather than the specific string: as far as we want the type-checker to be concerned, this is just some string, much like how a timeout duration would be just some number. It's not a string that we want to have participate in determining our types, like the tag strings that we put in the type property of a Message or an Action.

src/reduxTypes.js

src/realm/__tests__/realmReducer-test.js

gnprice · 2021-10-18T21:39:17Z

src/api/constants.js

+/**
+ * The topic servers understand to mean "there is no topic". This should
+ * match
+ *   https://github.com/zulip/zulip/blob/5.0-dev/zerver/lib/actions.py#L2714
+ * or similar logic at the latest `main`.
+ */


nit: one-line summary:

Suggested change

/**

* The topic servers understand to mean "there is no topic". This should

* match

* https://github.com/zulip/zulip/blob/5.0-dev/zerver/lib/actions.py#L2714

* or similar logic at the latest `main`.

*/

/**

* The topic servers understand to mean "there is no topic".

*

* This should match

* https://github.com/zulip/zulip/blob/5.0-dev/zerver/lib/actions.py#L2714

* or similar logic at the latest `main`.

*/

gnprice · 2021-10-18T21:50:46Z

src/compose/ComposeBox.js

  handleSend = () => {
-    const { dispatch } = this.props;
+    const { dispatch, mandatoryTopics, _ } = this.props;
    const { message } = this.state;
    const destinationNarrow = this.getDestinationNarrow();

+    if (
+      isTopicNarrow(destinationNarrow)
+      && topicOfNarrow(destinationNarrow) === apiConstants.NO_TOPIC_TOPIC
+      && mandatoryTopics
+    ) {
+      showErrorAlert(_('Message not sent'), _('Please specify a topic.'));
+      return;
+    }
+
    this.props.onSend(message, destinationNarrow);


So, this code actually runs not only when you send a message you've just composed in the normal way, but also when you hit save/submit on a message you've just gone and edited. (Maybe that's a sign we should rename this method, and also the onSend callback -- more like handleSubmit / onSubmit.)

How does this behave in the editing case? Seems like it'll give a confusing error message, if nothing else. What's even the desired semantics in that case -- if a message is already to (no topic), and then the setting is enabled, and then you go to edit it, can you edit it without changing its topic?

… I kind of don't want to let those questions block this PR, because that's a pretty rare case (editing is already uncommon, and editing with a target topic of (no topic) when that's forbidden is surely much more so), and because they seem like questions that may lead to a discussion of what the web app's behavior is and should be. Better to get the main issue fixed here, and leave that for another time.

But let's leave at least a TODO comment, like // TODO how should this behave in the isEditing case?. That way this at least doesn't much add to the confusingness of this code with respect to its reuse for editing vs. composing: the reader sees it's specifically about sending a message, but the comment warns that doesn't mean the function is only for the sending case.

chrisbobbe · 2021-10-19T19:03:00Z

Thanks for the review! Revision pushed.

We'll use this soon for enforcing the "require topics in stream messages" setting: https://zulip.com/help/require-topics. [chris: rewrote commit message, tweaked/relocated migration comment]

…ing.

Servers reject topics [1] (and, I would guess, streams too) if they are the empty string OR if they trim to the empty string. So, handle the trimming part. [1] See the comment added in the zulip-mobile commit e221fc1.

…erver code.

And be sure to send '(no topic)' to the server if no topic is provided. Servers reject topics that are the empty string or trim to the empty string; see the comment added in the zulip-mobile commit e221fc1.

(For stream messages, that is, not PMs.) Fixes: zulip#4378

gnprice · 2021-10-19T22:53:44Z

Thanks! Looks good; merging.

chrisbobbe · 2021-10-19T22:54:57Z

Thanks for the review!

thisisnitish · 2021-10-20T11:57:24Z

Thanks @chrisbobbe and @gnprice!, I was really worried about this issue, finally we did this. Thanks @chrisbobbe again for helping this much. Thanks 😊

chrisbobbe requested changes Jun 14, 2021

View reviewed changes

gnprice reviewed Jun 16, 2021

View reviewed changes

thisisnitish changed the title ~~Setup the 'realm_mandatory_topics' as 'isTopicMandatory' in Redux~~ Confirms that messages have topics, If there is "no topic" don't allow Jun 16, 2021

thisisnitish force-pushed the cannot_post_witout_topics branch from 5a43248 to 2e08d04 Compare June 16, 2021 18:31

thisisnitish force-pushed the cannot_post_witout_topics branch from 2e08d04 to 24cfc98 Compare July 1, 2021 18:29

thisisnitish changed the title ~~Confirms that messages have topics, If there is "no topic" don't allow~~ Compose area: Added the relevant property for topics Jul 1, 2021

chrisbobbe mentioned this pull request Jul 7, 2021

Fix do not mark as read, and make it more visible #4880

Merged

chrisbobbe force-pushed the cannot_post_witout_topics branch from fad95f0 to 69c026f Compare September 14, 2021 20:58

gnprice force-pushed the cannot_post_witout_topics branch from 69c026f to 2a87c77 Compare September 15, 2021 01:23

gnprice reviewed Sep 15, 2021

View reviewed changes

gnprice changed the title ~~Compose area: Added the relevant property for topics~~ Compose area: Enforce mandatory topics Sep 15, 2021

chrisbobbe force-pushed the cannot_post_witout_topics branch from 2a87c77 to f204d16 Compare October 18, 2021 21:03

chrisbobbe mentioned this pull request Oct 18, 2021

Enforce mandatory_topics setting #5055

Closed

gnprice reviewed Oct 18, 2021

View reviewed changes

chrisbobbe force-pushed the cannot_post_witout_topics branch from f204d16 to ae5945c Compare October 19, 2021 19:02

thisisnitish and others added 4 commits October 19, 2021 15:52

compose: Start storing realm_mandatory_topics value in Redux.

02839e0

We'll use this soon for enforcing the "require topics in stream messages" setting: https://zulip.com/help/require-topics. [chris: rewrote commit message, tweaked/relocated migration comment]

realmReducer tests: Start type-checking; use example data.

d5f4ca6

realmReducer tests: Add test for REALM_INIT.

c004235

ShareToStream [nfc]: Add a TODO to write down assumptions about a str…

a4a6fba

…ing.

chrisbobbe added 4 commits October 19, 2021 15:52

api/constants: Extract '(no topic)' to a constant; record source in s…

4788690

…erver code.

sharing: Stop requiring topics when not set as mandatory.

45d737e

And be sure to send '(no topic)' to the server if no topic is provided. Servers reject topics that are the empty string or trim to the empty string; see the comment added in the zulip-mobile commit e221fc1.

ComposeBox: Require topics if topics are set as mandatory.

2c3a94a

(For stream messages, that is, not PMs.) Fixes: zulip#4378

gnprice force-pushed the cannot_post_witout_topics branch from ae5945c to 2c3a94a Compare October 19, 2021 22:53

gnprice merged commit 2c3a94a into zulip:main Oct 19, 2021

gnprice mentioned this pull request Oct 19, 2021

chat: Fix create post without a topic #4543

Closed

Compose area: Enforce mandatory topics #4798

Compose area: Enforce mandatory topics #4798

Conversation

thisisnitish commented Jun 12, 2021 • edited by chrisbobbe

WesleyAC commented Jun 14, 2021

chrisbobbe left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe Jun 14, 2021 • edited

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

chrisbobbe Jun 14, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnprice 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

thisisnitish commented Jun 16, 2021

chrisbobbe commented Jun 16, 2021

thisisnitish commented Jun 16, 2021 • edited

chrisbobbe commented Jun 16, 2021 • edited

thisisnitish commented Jun 21, 2021 • edited

thisisnitish commented Jul 1, 2021

thisisnitish commented Jul 3, 2021

chrisbobbe commented Sep 14, 2021

gnprice 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

chrisbobbe commented Oct 18, 2021

gnprice 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

chrisbobbe commented Oct 19, 2021

gnprice commented Oct 19, 2021

chrisbobbe commented Oct 19, 2021

thisisnitish commented Oct 20, 2021 • edited

thisisnitish commented Jun 12, 2021 •

edited by chrisbobbe

chrisbobbe left a comment •

edited

chrisbobbe Jun 14, 2021 •

edited

chrisbobbe Jun 14, 2021 •

edited

thisisnitish commented Jun 16, 2021 •

edited

chrisbobbe commented Jun 16, 2021 •

edited

thisisnitish commented Jun 21, 2021 •

edited

thisisnitish commented Oct 20, 2021 •

edited