action sheet: add and implement delete topic option #3824

agrawal-d · 2020-01-18T15:47:16Z

agrawal-d · 2020-01-18T15:51:56Z

Flow issue I cant figure out : In line messageActionSheet.js:88, flow assumes message is of type Outbox even though the definition is message: Message | Outbox. This causes a flow error.

Is this fixable, or is thus a flow bug?

agrawal-d · 2020-01-18T16:05:12Z

@ray-kraesig @gnprice please review.

rk-for-zulip

Flow issue I cant figure out : In line messageActionSheet.js:88, flow assumes message is of type Outbox even though the definition is message: Message | Outbox. This causes a flow error.

Flow isn't assuming that message is of type Outbox; rather, you're assuming that message is of type Message. Flow is complaining (correctly, I believe) that you're trying to access a value that doesn't exist on Outbox.

The fix for this is going to be somewhat more complicated than mere syntax. (For example, it will have to correctly handle the case where the message whose header was selected was an Outbox, but has been successfully delivered while the user is still looking at the confirmation dialog.)

rk-for-zulip · 2020-01-21T00:56:30Z

src/message/messageActionSheet.js

+      {
+        text: 'Delete topic',
+        onPress: () => api.deleteTopic(auth, message.stream_id, message.subject),
+      },


This option needs to be marked as style: 'destructive' for iOS.

Also, it needs to report failure somehow (if #3795 is merged before this).

rk-for-zulip · 2020-01-21T00:58:04Z

src/message/messageActionSheet.js

@@ -79,6 +79,26 @@ const muteTopic = ({ auth, message }) => {
 };
 muteTopic.title = 'Mute topic';

+const deleteTopic = ({ auth, message }) => {
+  Alert.alert(
+    `Are you sure you want to delete the topic '${message.subject}' ?`,


This construction, and therefore this wording, won't work as is: it's impossible to localize properly.

(Since the topic is a fixed string that shouldn't be inflected in any language, we might be able to do something like

const alertTitle = _('Deleting topic: $topic').replaceAll('$topic', message.subject);

but that implies a can of worms I'm not willing to casually open. @gnprice?)

I'll wait for @gnprice 's response, before pushing any changes.

Erratum: this JS construct can't be localized, but the i18n library we have can help.

const alertTitle = _.intl.formatMessage( { id: "Are you sure you want to delete the topic '{topic}'?" }, { topic: message.subject }, );

The value of id is being taken literally, without substitution for topic. Using defaultMessage appears to work.

I referred to https://github.com/formatjs/react-intl/blob/master/docs/API.md#formatmessage

rk-for-zulip · 2020-01-21T01:00:26Z

static/translations/messages_en.json

@@ -50,6 +50,7 @@
  "Copy to clipboard": "Copy to clipboard",
  "Link copied to clipboard": "Link copied to clipboard",
  "Mute topic": "Mute topic",
+  "Delete topic":"Delete topic",


Space after the colon, for consistency.

agrawal-d · 2020-01-24T18:29:28Z

@ray-kraesig I'm not sure how to proceed with this, with respect to the flow issue and handling the corner cases you mentioned #3824 (review).
Can you please tell me in what I should do next to resolve that? Thanks!

rk-for-zulip · 2020-01-30T02:08:41Z

It's not really a Flow issue; it's a functional issue that Flow is recognizing. message may actually be an Outbox; that case will need to be tested for and handled separately, much like the delete-message callback does.

However, the delete-message action isn't a great template here, as it has the bug I mentioned: if a user long-presses a message before it's sent, then the delete-message callback will still attempt to delete it from the local Outbox (from which it's already been removed), rather than from the server, and no error will be given.

Resolving that (for either delete-message or delete-topic) will require some thought and some testing.

agrawal-d · 2020-02-06T17:11:38Z

src/topics/topicActions.js

+  dispatch: Dispatch,
+  getState: GetState,
+) => {
+  const messages: $ReadOnlyArray<Message | Outbox> = getMessagesForNarrow(getState(), narrow);


@ray-kraesig can you please help me out here? getMessagesForNarrow() is returning an empty array here, even though the narrow does contain several messages.
Output of the console statement ( line 44 ):

Am I using the selector incorrectly?

Posted a message on CZO here: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/.23M3824/near/815558

agrawal-d · 2020-02-08T18:23:20Z

@ray-kraesig have pushed some changes.

agrawal-d · 2020-02-11T15:17:12Z

Tests are failing, I will update them if the current change is acceptable.

andreasoc · 2020-02-11T15:27:16Z

sigh

rk-for-zulip

api.deleteTopic and deleteMessagesForTopic are self-contained enough that they could reasonably be moved to their own commit(s), to keep the size of this one down.

Please ensure tests pass before resubmitting for review.

rk-for-zulip · 2020-02-11T17:51:09Z

src/message/messageActionSheet.js

+  );
+  Alert.alert(
+    alertTitle,
+    'This will also delete all messages in the topic.',


This string also needs localization support.

rk-for-zulip · 2020-02-11T18:23:30Z

src/message/messageActionSheet.js

+    {
+      id: 'delete_topic_warning',
+      defaultMessage: "Are you sure you want to delete the topic '{topic}' ?",
+    },


defaultMessage and id should be identical.

id should be added to the localization database.

There should be no space before the question mark.

Formally, id is the localization database key, while defaultMessage is the message displayed if no message with key id is found in the localization database for the current language.

However, we require that defaultMessage be identical to the en database's entry for id (and, implicitly, that such an entry must exist). To ease the translation process, it's also our policy that id should be identical to these wherever feasible.

rk-for-zulip · 2020-02-11T18:26:24Z

src/message/messageActionSheet.js

+    'This will also delete all messages in the topic.',
+    [
+      {
+        text: 'Delete topic',


This string also needs localization support.

rk-for-zulip · 2020-02-11T19:13:43Z

src/message/messageActionSheet.js

+        onPress: async () => {
+          dispatch(deleteMessagesForTopic(getNarrowFromMessage(message, ownEmail), message)).catch(
+            err => {
+              Alert.alert(`${_(deleteTopic.errorMessage)} '${message.subject}'`, err.message);


Concatenating localized strings is essentially never safe. (Consider Japanese, where this would need to be something like 「{topic}」というトピックを削除できません.)

This would need to be a separately localized string... except that there shouldn't be any need for this interior Alert at all, because failure here should be propagated up to and handled by the new error-reporting mechanism in showActionSheet.

In order to do that, you'll effectively have to await on the Alert.alert – which isn't awaitable on its own, so you'll need to promisify it appropriately. See, e.g., here for a partial example.

rk-for-zulip · 2020-02-11T19:28:33Z

src/message/messageActionSheet.js

+        style: 'destructive',
+      },
+      {
+        text: 'Cancel',


This string also needs localization support.

agrawal-d · 2020-02-13T17:43:24Z

@ray-kraesig I've pushed some changes. I've also created a new test for the 'Delete topic' button.

agrawal-d · 2020-02-13T18:28:42Z

Please ensure tests pass before resubmitting for review.

Should tests always pass before requesting a review?
Is it ever okay if tests are not passing when requesting an initial review? I'm asking because the initial review may result in significant changes requiring the tests to be changed again.

rk-for-zulip · 2020-02-14T02:18:27Z

Should tests always pass before requesting a review?

Almost always, yes. (Although see below.)

Is it ever okay if tests are not passing when requesting an initial review? I'm asking because the initial review may result in significant changes requiring the tests to be changed again.

That is sometimes the case! However, when it is, the review request should always be accompanied with an explanation of why the tests don't pass, and what questions you need to have answered in order to fix them. Otherwise, I'm probably going to infer that you're still working on the PR, and that it's not actually ready for review.

Putting off fixing the tests until you get a first review may be appropriate when those fixes would be a significant and potentially wasteful undertaking in themselves. (Although one would definitely still need a comment stating and explaining this.) However, in this case:

Fixing the tests is straightforward – not quite as straightforward as it should be (cleanup is badly needed!), but still, not terribly intrusive.
Essentially any implementation of a Delete topic button-insertion test is going to need to know whether the user is an admin. Even a total rewrite would be very unlikely to change that.

rk-for-zulip

If we're going around again, one minor fix to the code itself:

rk-for-zulip · 2020-02-25T01:08:04Z

src/message/messageActionSheet.js

+        [
+          {
+            text: _('Delete topic'),
+            onPress: async () => {


This shouldn't need to be async. (Compare with the other branch.)

agrawal-d · 2020-02-25T07:22:45Z

@gnprice I'm having some trouble converting messageActionSheet-test.js to a type checked one.

I added // @flow strict-local to the top.
This causes the following error in several places:

For:
const store = mockStore({ ...

Cannot call `mockStore` with object literal bound to `middlewares` because  object literal [1] is incompatible with  array type [2].Flow(InferError)
messageActions-test.js(14, 31): [1] object literal
redux-mock-store_v1.2.x.js(26, 42): [2] array type

I think flow is not not using our mockStore defined in redux-mock-store.js and instead usinng redux-mock-store_v1.2.x.js to infer the type - moving the contents of redux-mock-store.js into messageActions-test.js somehow fixes this error.

After I temporarily fixed the error in step 2, I get several more errors of these two types:

For :
store.dispatch(doNarrow(streamNarrowObj));

Cannot call `store.dispatch` with `doNarrow(...)` bound to `action` because property `type` is missing in  function [1] but exists in  object type [2].Flow(InferError)
messagesActions.js(24, 83): [1] function
redux-mock-store_v1.2.x.js(14, 28): [2] object type

For :
expect(actions[0].type).toBe('DO_NARROW');

Cannot get `actions[0].type` because property `type` is missing in  function [1].Flow(InferError)

I tried a lot, but couldn't think of how to resolve these :'(

agrawal-d · 2020-02-25T16:45:23Z

@ray-kraesig I've removed the async keyword in the latest force-push.

rk-for-zulip · 2020-02-26T19:17:44Z

@gnprice I'm having some trouble converting messageActionSheet-test.js to a type checked one.

The lines of code quoted here appear to be from messageActions-test.js, not messageActionSheet-test.js. That file's a little out of scope for this PR, I think. 🙂

agrawal-d · 2020-02-26T19:24:32Z

The lines of code quoted here appear to be from messageActions-test.js, not messageActionSheet-test.js. That file's a little out of scope for this PR, I think. slightly_smiling_face

Oh, this is hilarious! Thanks!

I spent such a long time on trying to fix that file :'(

agrawal-d · 2020-02-26T21:44:11Z

@gnprice @ray-kraesig I've made all the changes - the tests are type-checked now. Can you please review this? Thanks!

agrawal-d · 2020-02-26T21:59:00Z

Not sure why the ordering of commits is different in GitHub UI, here's the output of
git log --pretty=oneline --abbrev-commit

0bf5b440 (HEAD -> delete-topic, agrawal-d/delete-topic) action sheet: Add delete topic feature.
4227e799 topic actions: Create action 'deleteMessagesForTopic'.
fc435354 api: Create API function for 'delete topic'.
acf3fb75 messageActionSheet [nfc]: Refactor tests to make them type-checked.
51be05fe exampleData: Add sample data of types 'BackgroundData' and 'Narrow'.

chrisbobbe · 2020-02-26T22:23:33Z

Not sure why the ordering of commits is different in GitHub UI

Well spotted! 🙂 Greg explained this to me here; it's a bug in GitHub where they're sorting by author date instead of committer date. I think we have this documented somewhere, but I haven't found it in a quick search on https://zulip.readthedocs.io/en/latest/overview/index.html, where most of our git-related documentation lives.

rk-for-zulip

A few nitpicks about data location. (Also, there are merge conflicts, so a rebase is needed anyway.)

rk-for-zulip · 2020-02-28T01:54:13Z

src/__tests__/exampleData.js

@@ -137,6 +138,13 @@ const displayRecipientFromUser = (user: User): PmRecipientUser => {
  });
 };

+export const narrow: Narrow = [


There's probably no benefit in having a Narrow here; pretty much every test that needs a Narrow needs to specify what sort of Narrow it's using.

In particular, this is true of your tests in mAS-test.js, which actually assume that they're getting a Narrow with this structure and these values: if this were replaced with, e.g. [{operator: 'is', operand: 'starred'}], those tests would break. That makes this not "example data" – it's test-specific data, and should go alongside the tests it's a part of.

You're right - thanks for the explanation!

rk-for-zulip · 2020-02-28T01:55:19Z

src/__tests__/exampleData.js

+    historical: {},
+    is_me_message: {},
+  },
+  mute: [['announce', 'stream events']],


Also test-specific data. (Probably belongs in baseBackgroundData.)

rk-for-zulip · 2020-02-28T02:06:26Z

src/message/__tests__/messageActionSheet-test.js

+      ...eg.streamMessage(),
      id: 3,
      reactions: [],
+      avatar_url: '',
+      client: '',


Since you're using eg.streamMessage() now, you can just do const message = eg.streamMessage({ id: 3 }) here (and similarly elsewhere) – there's no need to explicitly specify any properties which aren't relevant to the test.

rk-for-zulip · 2020-02-28T02:18:48Z

src/__tests__/exampleData.js

+  },
+  mute: [['announce', 'stream events']],
+  ownUser: selfUser,
+  subscriptions: [


agrawal-d · 2020-02-28T13:02:12Z

@ray-kraesig thank you for the review. I have made the changes you requested. Please review it again.

agrawal-d · 2020-02-28T13:03:39Z

Greg explained this to me here; it's a bug in GitHub where they're sorting by author date instead of committer date.

Oh - that's interesting. Thank's @chrisbobbe !

We are moving towards making all our tests type-checked. This commit is a move in that direction.

The 'delete topic' endpoint allows for the deletion of all messages in a stream for a given topic.

'deleteMessagesForTopic' deletes all messages (including those in outbox) for a given topic in a stream.

The webapp offers the feature to delete all messages within a topic for a stream. This feature is missing in the mobile app. Add 'Delete topic' button in message action sheet. Fixes zulip#3806.

rk-for-zulip

To avoid another round-trip (over the weekend, at that) with only trivial changes, I've appended a couple of commits to this PR. The first addresses the issue below; the second was a suggestion from @gnprice when reviewing the former.

rk-for-zulip · 2020-02-29T02:03:49Z

src/topics/topicActions.js

@@ -34,3 +36,25 @@ export const fetchTopicsForActiveStream = (narrow: Narrow) => async (
  }
  dispatch(fetchTopics(stream.stream_id));
 };
+
+export const deleteMessagesForTopic = (narrow: Narrow, message: Message | Outbox) => async (


I should have noticed this during one of the preceding passes, but – why does this function take a Message | Outbox? All the information it needs is in the Narrow.

(For that matter, it would be fairly simple just to pass the stream name and topic as parameters – there's no need to wrap them up in a Narrow object which you then have to check, or assume, to be of the right shape.

Narrow should be a type we'd want here. Unfortunately it's really a server API type that we've pressed into service for insufficiently-closely-related purposes in the mobile app. 😞 We've been discussing its replacement for some time.)

`deleteMessagesForTopic` currently takes both a `Narrow` and a `Message | Outbox`. However, this is redundant; all the relevant information can be obtained from either one. Furthermore, neither is really necessary: we can just ask the caller to extract the data and hand it to us. Since the caller already knows that it's operating on a `stream` message, it's actually less trouble than constructing the `Narrow`.

Outbox messages of type `private` will not have a comparable `.display_recipient`, and will have no `.subject` at all. While the comparisons currently performed are harmless and will, in this case, yield the right result, it's cleaner conceptually (and slightly safer when refactoring) to actually check the type first.

rk-for-zulip · 2020-02-29T02:52:45Z

And merged. Thanks!

agrawal-d requested review from gnprice and rk-for-zulip January 18, 2020 15:47

agrawal-d mentioned this pull request Jan 18, 2020

Feature request: delete topics #3806

Closed

agrawal-d force-pushed the delete-topic branch from 7e51546 to d7f26a0 Compare January 18, 2020 16:01

rk-for-zulip reviewed Jan 21, 2020

View reviewed changes

agrawal-d removed the request for review from gnprice February 3, 2020 06:29

agrawal-d force-pushed the delete-topic branch from d7f26a0 to 1b112d8 Compare February 6, 2020 17:09

agrawal-d changed the title ~~action sheet: add and implement delete topic option~~ [WIP] action sheet: add and implement delete topic option Feb 6, 2020

agrawal-d commented Feb 6, 2020

View reviewed changes

agrawal-d force-pushed the delete-topic branch from 1b112d8 to 4580d95 Compare February 8, 2020 17:53

agrawal-d changed the title ~~[WIP] action sheet: add and implement delete topic option~~ action sheet: add and implement delete topic option Feb 8, 2020

agrawal-d requested a review from rk-for-zulip February 8, 2020 17:54

agrawal-d force-pushed the delete-topic branch 2 times, most recently from b366b10 to 620a85c Compare February 8, 2020 18:18

agrawal-d force-pushed the delete-topic branch from 620a85c to f8cdeaa Compare February 8, 2020 18:30

rk-for-zulip reviewed Feb 11, 2020

View reviewed changes

agrawal-d force-pushed the delete-topic branch 2 times, most recently from 61d97a5 to c1243e8 Compare February 13, 2020 17:37

agrawal-d requested a review from rk-for-zulip February 13, 2020 17:42

agrawal-d force-pushed the delete-topic branch from c1243e8 to 0a959bc Compare February 13, 2020 17:45

rk-for-zulip reviewed Feb 25, 2020

View reviewed changes

agrawal-d force-pushed the delete-topic branch from 5ace59f to 4b1f264 Compare February 25, 2020 16:42

agrawal-d force-pushed the delete-topic branch 3 times, most recently from d19a024 to 0bf5b44 Compare February 26, 2020 21:39

agrawal-d requested a review from rk-for-zulip February 26, 2020 21:48

rk-for-zulip reviewed Feb 28, 2020

View reviewed changes

agrawal-d force-pushed the delete-topic branch from 0bf5b44 to 5b3acf4 Compare February 28, 2020 13:01

agrawal-d requested a review from rk-for-zulip February 28, 2020 13:02

agrawal-d added 4 commits February 28, 2020 18:34

messageActionSheet [nfc]: Refactor tests to make them type-checked.

3c531ce

We are moving towards making all our tests type-checked. This commit is a move in that direction.

api: Create API function for 'delete topic'.

7b2353c

The 'delete topic' endpoint allows for the deletion of all messages in a stream for a given topic.

topic actions: Create action 'deleteMessagesForTopic'.

2bb9cec

'deleteMessagesForTopic' deletes all messages (including those in outbox) for a given topic in a stream.

action sheet: Add delete topic feature.

b886b3d

The webapp offers the feature to delete all messages within a topic for a stream. This feature is missing in the mobile app. Add 'Delete topic' button in message action sheet. Fixes zulip#3806.

agrawal-d force-pushed the delete-topic branch from 5b3acf4 to b886b3d Compare February 28, 2020 13:06

rk-for-zulip reviewed Feb 29, 2020

View reviewed changes

rk-for-zulip added 2 commits February 28, 2020 18:45

rk-for-zulip force-pushed the delete-topic branch from b4135c7 to ad1556b Compare February 29, 2020 02:45

rk-for-zulip merged commit 8b005b8 into zulip:master Feb 29, 2020

gnprice mentioned this pull request Mar 3, 2020

Extend exampleData to further clean up messageActionSheet tests #3946

Merged

action sheet: add and implement delete topic option #3824

action sheet: add and implement delete topic option #3824

Conversation

agrawal-d commented Jan 18, 2020 • edited Loading

agrawal-d commented Jan 18, 2020 • edited Loading

agrawal-d commented Jan 18, 2020

rk-for-zulip 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

agrawal-d commented Jan 24, 2020

rk-for-zulip commented Jan 30, 2020

agrawal-d Feb 6, 2020 • edited Loading

Choose a reason for hiding this comment

agrawal-d Feb 8, 2020 • edited Loading

Choose a reason for hiding this comment

agrawal-d commented Feb 8, 2020

agrawal-d commented Feb 11, 2020

andreasoc commented Feb 11, 2020

rk-for-zulip 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

agrawal-d commented Feb 13, 2020 • edited Loading

agrawal-d commented Feb 13, 2020

rk-for-zulip commented Feb 14, 2020

rk-for-zulip left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agrawal-d commented Feb 25, 2020 • edited Loading

agrawal-d commented Feb 25, 2020

rk-for-zulip commented Feb 26, 2020

agrawal-d commented Feb 26, 2020 • edited Loading

agrawal-d commented Feb 26, 2020

agrawal-d commented Feb 26, 2020

chrisbobbe commented Feb 26, 2020 • edited Loading

rk-for-zulip 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

agrawal-d commented Feb 28, 2020

agrawal-d commented Feb 28, 2020

rk-for-zulip left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rk-for-zulip commented Feb 29, 2020

agrawal-d commented Jan 18, 2020 •

edited

Loading

agrawal-d commented Jan 18, 2020 •

edited

Loading

agrawal-d Feb 6, 2020 •

edited

Loading

agrawal-d Feb 8, 2020 •

edited

Loading

agrawal-d commented Feb 13, 2020 •

edited

Loading

agrawal-d commented Feb 25, 2020 •

edited

Loading

agrawal-d commented Feb 26, 2020 •

edited

Loading

chrisbobbe commented Feb 26, 2020 •

edited

Loading