-
-
Notifications
You must be signed in to change notification settings - Fork 643
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add mark as read option in TopicActionSheet and Modify TopicActionSheet to use stream_id
.
#4635
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Looks good overall, just a few changes on the stream name/stream ID refactor :)
src/message/messageActionSheet.js
Outdated
const unmuteTopic = async ({ auth, stream, topic }) => { | ||
await api.unmuteTopic(auth, stream, topic); | ||
const unmuteTopic = async ({ auth, streamId, topic, subscriptions }) => { | ||
const sub = subscriptions.find(x => x.stream_id === streamId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of adding this code, just change api.unmuteTopic
and api.muteTopic
to take a stream ID. I'd do that in the commit before this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
src/message/messageActionSheet.js
Outdated
@@ -167,34 +172,34 @@ const deleteTopic = async ({ auth, stream, topic, dispatch, _ }) => { | |||
); | |||
}); | |||
if (await AsyncAlert()) { | |||
await dispatch(deleteMessagesForTopic(stream, topic)); | |||
const sub = subscriptions.find(x => x.stream_id === streamId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be fairly simple to rewrite deleteMessagesForTopic
to take a stream ID — do that instead of finding the stream name here. Again, I'd do that in a commit before this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, did the necessary changes.
src/message/messageActionSheet.js
Outdated
@@ -427,12 +432,17 @@ export const showTopicActionSheet = ({ | |||
unreadStreams: UnreadStreamsState, | |||
... | |||
}>, | |||
stream: string, | |||
streamId?: number, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should also not be optional. I suspect it may be simpler to keep this taking a stream: string
for now, so that we can centralize the error handling (since currently, both of the callers get the stream id by searching the subscriptions), but I don't have a strong feeling on that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
using streamId
does cause me to repeat the error handling code at the calling site. But If we use stream
we would have a mixture of two types of ActionSheet (one wanting stream and one wanting streamId) since there are many places where I can easily use StreamId
(without searching). If we don't do this mixup we would be promoting the use of stream
where we could easily have used streamId
which may make using showTopicActionSheet
difficult in some new places or it may make the refactor we would eventually do, difficult.
This is not a very convincing argument by me but I am still using streamId
.
Hmm, I just tested this on my device, and pressing "Mark topic as read" from the home screen doesn't seem to do anything (the topic is still unread). Any idea what's up with that? It seems like the server gets that it should be marked as read (they're marked as read on the webapp), but the main screen doesn't re-render. If I kill the app and reopen it, they do disappear as well, though. |
I am speculating that this was related to syncing read/unread state. @WesleyAC This PR is ready for a re-review, I had a bit of doubt regarding [nfc] commits It would be great if you could clear that -- Are we ever going to do the Outbox refactor to work along with stream_id, if so would it be a good idea to also add TODOs at places that were changed in this PR but would require another change once the Outbox refactor is done, for example at the call site of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good! Left a few comments about your recent changes.
Oh, and your |
Thanks for the review @WesleyAC, made the changes you just requested. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @AkashDhiman! See a few comments below, most of them small.
There's one thing I'm hesitating on before merging: #3244 (comment). It'd be great to have @gnprice's and/or @WesleyAC's thoughts on whether this is an area where we can drop support for servers 1.9.x and below, or if we should write a conditional to keep compatibility, or if we should do more thinking and work about establishing what servers are officially supported, and how to communicate that, first. Discussion in chat.
Also, I notice that there are some commits where the commit message body goes past 70 columns in a line; let's wrap those to be in line with the style guide:
Any paragraph content in the commit message should be line-wrapped to about 68 characters per line, but no more than 70, so that your commit message will be reasonably readable in
git log
in a normal terminal. You may find it helpful to […]
The summary line can go a bit longer just because it has to, sometimes; that guide says no more than 76 characters for the summary line.
src/message/messageActionSheet.js
Outdated
if (sub) { | ||
await api.setTopicMute(auth, sub.stream_id, topic, false); | ||
} | ||
const unmuteTopic = async ({ auth, streamId, topic, subscriptions }) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: subscriptions
not needed here
@@ -5,12 +5,12 @@ import { apiPatch } from '../apiFetch'; | |||
/** See https://zulip.com/api/mute-topic */ | |||
export default async ( | |||
auth: Auth, | |||
stream: string, | |||
streamId: number, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
api [nfc]: Update setTopicMute to use stream_id.
This commit also involves appropriate changes to the code calling
`setTopicMute`.
Fixes: #3244
Glad that this fixes #3244! 🙂 Let's include that in the PR description as a hint for GitHub to fill in the metadata.
I'm not sure offhand if this implies that #3918 is fixed...I've just posted about that at #3918 (comment). 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also posted a point at #3244 (comment) that may need some separate work / thinking to resolve.
src/message/messageActionSheet.js
Outdated
await dispatch(deleteMessagesForTopic(stream, topic)); | ||
const sub = subscriptions.find(x => x.name === stream); | ||
if (sub) { | ||
await dispatch(deleteMessagesForTopic(sub.stream_id, topic)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the added if (sub)
conditional a functional change? If so, let's be sure it's a change that we want, describe it in the commit message, and un-mark the commit as NFC.
If it is NFC, I'm not seeing that that's obvious; that should get an explanation if it turns out to be the case, I think. 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, looking back after posting the review: if it turns out to be really clear that we expect sub
to exist every single time, this is probably another good place to use invariant
(see my other comments where I mention that).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using invariant in the new revision,
I think subscriptions
will not change at this position i.e. it will keep using the same value that showTopicActionSheet
was called with. even if it is updated by react in between the execution of this function.
I am not sure if that is intentional though (since such a change can happen when the user is about to interact with the action sheet.)
So if showTopicActionSheet
is called with right stream name and Subscription object here, (sub !== undefined)
will always be true.
Hence I am using invariant.
Why i think subscriptions
will not change at this position?
I think this is an example of "stale closure" (more info at: https://reactjs.org/docs/hooks-faq.html#why-am-i-seeing-stale-props-or-state-inside-my-function)
Essentially what I could infer from it was that when we pass a state to a function and the state updates, our function creates a closure with the value of state that was passed to it when the function was called and it keeps using that outdated data even when the state updates.
Just in case, a way to solve this is by using useRef
(for functional components) and its equivalent for class. Here is a blog post discussing this: https://css-tricks.com/dealing-with-stale-props-and-states-in-reacts-functional-components/
src/__tests__/lib/exampleData.js
Outdated
@@ -225,12 +225,12 @@ export const crossRealmBot: CrossRealmBot = makeCrossRealmBot({ name: 'bot' }); | |||
|
|||
const randStreamId: () => number = makeUniqueRandInt('stream IDs', 1000); | |||
export const makeStream = ( | |||
args: {| name?: string, description?: string |} = Object.freeze({}), | |||
args: {| name?: string, description?: string, streamId?: number |} = Object.freeze({}), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Several of these functions that make data objects (streamMessage
, pmMessage
, etc.) accept the property names directly, without bothering to use camelCase. Best to do so here, for consistency, I think: so, stream_id
instead of streamId
.
src/message/messageActionSheet.js
Outdated
topic: string, | ||
|}): void => { | ||
const sub = backgroundData.subscriptions.find(x => x.stream_id === streamId); | ||
if (!sub) { | ||
logging.error('Stream id does not exist.'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this quite the right error message? We've got a stream ID; it's the subscription that we haven't found, right?
Also, do we expect the if (!sub)
condition to ever happen, and why or why not?
If we never expect it to happen, let's use invariant
instead (here and in similar places in this commit); the number of places we've used that is small but growing. 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using invariant in the new revision. The reason is explained at #4635 (comment)
Also I changed the error message in invariant
.
src/streams/TopicItem.js
Outdated
return ( | ||
<Touchable | ||
onPress={() => onPress(stream, name)} | ||
onLongPress={() => { | ||
if (!subscription) { | ||
logging.error('Stream id does not exist.'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(same comment as above, about the error message, whether we expect this to happen, and invariant
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On exploring this I realized that the current implementation allows a user to Long Press TopicItem
on an unsubscribed stream (from TopicListScreen
) (I think ideally this shouldn't be the case).
In that scenario subscription will be undefined.
Given that case I am not sure if we should be logging.error
as well, so I removed that statement in the new revision and now the code simply returns on not finding subscription.
src/webview/handleOutboundEvents.js
Outdated
const stream = streamNameOfStreamMessage(message); | ||
const subscription = backgroundData.subscriptions.find(x => x.name === stream); | ||
|
||
if (!subscription) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(same comment as above, about whether we expect this to happen, and invariant
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
using invariant here, the reason is same as #4635 (comment)
const markTopicAsRead = async ({ auth, streamId, topic }) => { | ||
await api.markTopicAsRead(auth, streamId, topic); | ||
}; | ||
markTopicAsRead.title = 'Mark topic as read'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I meant to post in my previous review but forgot:
The title and error message are translated via _
(the function with type GetText
), so we should make sure these user-facing strings have entries in static/translations/messages_en.json; looks like 'Mark topic as read' is already there but the error message is not.
2e0b7c9
to
44788c2
Compare
@chrisbobbe Thanks for the review, I think I have addressed your comments and you may review the changes again. Please pay particular attention to #4635 (comment), as I highlight what seems to me as something that can be considered an issue. |
src/message/messageActionSheet.js
Outdated
|}): Button<HeaderArgs>[] => { | ||
|}): Button<TopicArgs>[] => { | ||
const sub = subscriptions.find(x => x.stream_id === streamId); | ||
invariant(sub, 'Subscription with provided stream id does not exist.'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just highlighting, that I have used an invariant here as well (this wasn't asked in @chrisbobbe's review). necessary for the subsequent call to isTopicMuted
.
for the same reason as #4635 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, writing this expectation explicitly like this is helpful. I think the expectation is not correct. 😁 I'll elaborate in a review comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(here's a comment just to tie this review to this thread)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @AkashDhiman for these changes, and @WesleyAC and @chrisbobbe for the reviews!
I haven't read all of this in detail, but one thing I see is that this code appears to assume that for any topic we validly try to make a topic action sheet for, there'll be an entry in subscriptions
for the stream. I think that isn't right, because we'll only have one of those if the user is subscribed to the stream. For information about the stream in general, not specific to the user's individual relationship to the stream, we want a Stream
value, gotten from getStreamsById
or its relatives.
Here's a general sequence of changes that I think would work well. Each of these might be one or several commits.
- (Before anything else, the rename to "topic" from "header". That is overdue 😉 .)
- Then, first, have the action-sheet code take a stream ID as well as a stream name, redundantly.
- Can call them
streamName
andstreamId
. - The caller can look the ID up, if it doesn't have it, in
getStreams(state)
(just like the code in the current revision of this PR looks in the result ofgetSubscriptions(state)
.) Then this code doesn't need to do that, and the presence of stream IDs can be pushed further upstream in later followups.
- Can call them
- Then, make the
setTopicMute
anddeleteMessagesForTopic
refactors -- but this time without thesubscriptions.find
, and instead using the stream ID we were passed. - Now, make the changes supplying the unreads data and adding the option to mark as read.
- Meanwhile, if nothing is using the stream name anymore, stop passing it in. Or if something is, perhaps refactor that away too as a followup.
- Can switch from
subscriptions: getSubscriptions(state)
in the background data tosubscriptionsById: getSubscriptionsById(state)
. - Can also add
streamsById: getStreamsById(state)
, if that helps.
- Can switch from
src/message/messageActionSheet.js
Outdated
|}): Button<HeaderArgs>[] => { | ||
|}): Button<TopicArgs>[] => { | ||
const sub = subscriptions.find(x => x.stream_id === streamId); | ||
invariant(sub, 'Subscription with provided stream id does not exist.'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(here's a comment just to tie this review to this thread)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's also some comments from looking specifically at the test code. Thanks again!
src/__tests__/lib/exampleData.js
Outdated
/** Construct mock UnreadStreamsState. */ | ||
export const makeUnreadStreamsState = ( | ||
mockData: {| | ||
streamId: number, | ||
topics: {| | ||
topicName: string, | ||
messageIds: number[], | ||
|}[], | ||
|}[], | ||
): UnreadStreamsState => | ||
Immutable.Map( | ||
mockData.map(unreadStream => [ | ||
unreadStream.streamId, | ||
Immutable.Map( | ||
unreadStream.topics.map(unreadTopic => [ | ||
unreadTopic.topicName, | ||
Immutable.List(unreadTopic.messageIds), | ||
]), | ||
), | ||
]), | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of directly building up the internal data structure that state.unread.streams
comes in, let's use the unreads reducer to build it up. That way:
- The details of the data structure stay internal to the unreads model. This makes it easier to change that data structure around later (as we recently did, in fact, earlier this year.)
- By keeping those details out, it also keeps this code that's building the data more abstract in a way that I think will make it more readable: it'll say "start from an initial/empty state, then insert each of the messages in this list".
- Because the reducer takes basically message objects, I think this will ultimately help simplify the tests that use this, too: instead of referring to a stream by an ID here and a name there, and counting on tying those together elsewhere in the test, or using the same ID or name in several places and implicitly using that they're the same value, they can use a real
Stream
object throughout.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For an example, see:
zulip-mobile/src/unread/__tests__/unreadModel-test.js
Lines 149 to 160 in 31c4bc8
const streamAction = args => mkMessageAction(eg.streamMessage(args)); | |
const baseState = (() => { | |
const r = (state, action) => reducer(state, action, eg.plusReduxState); | |
let state = initialState; | |
state = r(state, streamAction({ stream_id: 123, subject: 'foo', id: 1 })); | |
state = r(state, streamAction({ stream_id: 123, subject: 'foo', id: 2 })); | |
state = r(state, streamAction({ stream_id: 123, subject: 'foo', id: 3 })); | |
state = r(state, streamAction({ stream_id: 234, subject: 'bar', id: 4 })); | |
state = r(state, streamAction({ stream_id: 234, subject: 'bar', id: 5 })); | |
return state; | |
})(); |
For another example:
zulip-mobile/src/unread/__tests__/unread-testlib.js
Lines 36 to 58 in 31c4bc8
let state = initialState; | |
for (const message of [ | |
eg.streamMessage({ stream_id: 0, subject: 'a topic', id: 1, flags: ['mentioned'] }), | |
eg.streamMessage({ stream_id: 0, subject: 'a topic', id: 2, flags: ['mentioned'] }), | |
eg.streamMessage({ stream_id: 0, subject: 'a topic', id: 3, flags: ['mentioned'] }), | |
eg.streamMessage({ stream_id: 0, subject: 'another topic', id: 4 }), | |
eg.streamMessage({ stream_id: 0, subject: 'another topic', id: 5 }), | |
eg.streamMessage({ stream_id: 2, subject: 'some other topic', id: 6 }), | |
eg.streamMessage({ stream_id: 2, subject: 'some other topic', id: 7 }), | |
eg.pmMessageFromTo(user0, [user1], { id: 11 }), | |
eg.pmMessageFromTo(user0, [user1], { id: 12 }), | |
eg.pmMessageFromTo(user2, [user1], { id: 13 }), | |
eg.pmMessageFromTo(user2, [user1], { id: 14 }), | |
eg.pmMessageFromTo(user2, [user1], { id: 15 }), | |
eg.pmMessageFromTo(user2, [user1, user3], { id: 21 }), | |
eg.pmMessageFromTo(user2, [user1, user3], { id: 22 }), | |
eg.pmMessageFromTo(user4, [user1, user5], { id: 23 }), | |
eg.pmMessageFromTo(user4, [user1, user5], { id: 24 }), | |
eg.pmMessageFromTo(user4, [user1, user5], { id: 25 }), | |
]) { | |
state = reducer(state, mkMessageAction(message), globalState); | |
} | |
return state; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of directly building up the internal data structure that
state.unread.streams
comes in, let's use the unreads reducer to build it up.
(This isn't a principle we have written down anywhere -- in fact I really only started doing it in our code just recently, with that refactor to this very unreads model. I think it becomes more important as our data structures get interesting, like that one is. @chrisbobbe noticed it at the time and we had a conversation about it, which might be informative to read and I should perhaps turn into a style-guide entry or something; maybe it was on one of the PRs where I was making those changes?)
const stream = eg.makeStream({ name: 'test stream', stream_id: 123 }); | ||
const subscriptions = [{ ...eg.subscription, ...stream }]; | ||
const unreadStreams = eg.makeUnreadStreamsState([ | ||
{ streamId: 123, topics: [{ topicName: 'test topic', messageIds: [1, 2] }] }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of saying streamId: 123
here and stream_id: 123
above where stream
is constructed, this can say streamId: stream.stream_id
here. That way the connection between this and the ID of stream
is explicit.
I think that also makes it unnecessary to add a stream_id
option to eg.makeStream
. In general I think in almost any place we might use that, it'd be better to turn the flow around just like here: let the stream get whatever ID it gets, and where we want to have that stream's ID just refer to it directly.
describe('constructHeaderActionButtons', () => { | ||
describe('constructTopicActionButtons', () => { | ||
test('show mark as read if topic is unread', () => { | ||
const stream = eg.makeStream({ name: 'test stream', stream_id: 123 }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly, instead of explicitly setting name: 'test stream'
and then writing test stream
again later in the test (in one of the intermediate commits in this branch), we can just say stream.name
at that later point.
That makes the test code simpler and shorter -- and in particular it makes it clear that there's nothing interesting about the particular name and it isn't something the reader needs to pay attention to.
That at that point we're passing no arguments to eg.makeStream
at all, so we might simplify further by using eg.stream
instead of a local, to further make clear that there's nothing interesting about this particular stream.
const buttons = constructTopicActionButtons({ | ||
backgroundData: { ...baseBackgroundData, subscriptions, unreadStreams }, | ||
streamId: stream.stream_id, | ||
topic: 'test topic', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In a related spirit, instead of saying the string 'test topic'
twice and implicitly using that they're the same, it'd be better to have a const topic = 'test topic'
and refer to that in both places.
08d39d0
to
7c39641
Compare
@gnprice thank you for the review, I have made the changes you requested.
Yes I did make this assumption in the previous revision (changed now). It felt a bit unnatural to me that we provide options like mute topic to a topic in a stream we aren't really subscribed to and it could also cause conflicts with future options that would be even more irrelevant for topics in muted streams. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @AkashDhiman for the revision! About to go to bed, but I've read the first 7 commits:
28d86d0 ActionSheet [nfc]: Rename HeaderActionSheet to TopicActionSheet.
5f1cc4a ActionSheet [nfc]: Rename variable stream
to streamName
.
4e88484 ActionSheet [nfc]: Pass StreamsById
to actionsheet.
bf6add0 ActionSheet [nfc]: Parameterize some values used in tests
.
f44af8b ActionSheet [nfc]: Update signature of actionsheet to use streamId.
288edc1 ActionSheet [nfc]: Use streamId in actionsheet functions wherever possible.
2fb84bb api [nfc]: Update setTopicMute
to use stream_id
.
and they generally look great. Comments below.
That leaves 4 further commits:
0206d89 topicActions [nfc]: Use streamId
in deleteMessagesForTopic
.
5b0bf1b ActionSheet [nfc]: Pass unreadStreams data to action sheet.
9e80f62 ActionSheet: Add mark as read option to topic action sheet.
7c39641 ActionSheet [nfc]: Update signature of actionsheet to not use streamName
.
which I'll look at later.
apiPatch(auth, 'users/me/subscriptions/muted_topics', { | ||
stream, | ||
stream_id: streamId, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will change what interface we're assuming from the server when we make a request there. When we do that, we want to be sure to say in the commit message why we believe the new interface will work.
Following the link to #3244, it looks like this new way is accepted starting with server 2.0. We're planning to desupport servers older than that soon, but we're not quite ready to do so -- #4750 added a banner to warn people on such old servers that it's time to upgrade, and we'll want to have the release with that banner out for a bit before having a further release that actually stops supporting the older servers.
So let's hold this commit off for now, leaving it out of this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, and read the remaining commits! All look good, modulo small comments below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @AkashDhiman for the revision! All looks good in the code except one item mentioned below. Please go ahead and fix that, and then we can merge 😄
Meanwhile, here's a nit in a commit message and a tip:
ActionSheet [nfc]: Update signature of actionsheet to not use
`streamName`.
That's a long summary line. The first thing you can do to shorten it is to drop the second mention of "action sheet" -- the prefix already tells the reader that that's the area we're talking about. So:
ActionSheet [nfc]: Update signature to not use
streamName
.
Then I'd reword with a verb that lets us get faster to the point. Here, "drop" carries the meaning of "update … to not use". So we can say:
ActionSheet [nfc]: Drop
streamName
from signature.
A bonus of that wording is that it gets the "streamName
" bit earlier in the summary line, and that's the part that's most specific to this change.
src/streams/TopicItem.js
Outdated
showActionSheetWithOptions, | ||
callbacks: { dispatch, _ }, | ||
backgroundData, | ||
stream, | ||
streamId: stream[0], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you want stream.stream_id
here 😉
(And in the other spot like it.)
@gnprice done. |
This is a better name since this type of action sheet is used with Topics.
`streamName` is a better name as it makes it more explicit that it contains the value of stream's name and not a `Stream` object. This name will also be consistent (from a readability point of view) with variables `streamId` and `streams` that will be introduced in subsequent commits.
While not directly used within the action sheet, this modification will help us to create TopicActionSheet with `streamId` for any `streamName` at the callsite of `showTopicActionSheet`.
This is better since most of these things are / can-be reused in the test suite of concern, and it is easy to change their values should we choose to do so.
Passed to background data of TopicItem and MessageList, to use in later commit.
This is to facilitate complete migration to using stream id for actionsheet functions instead of stream name.
…sible. This commits uses the streamId parameter (introduced in the previous commit) to replace the usage of `streamName` as stream identifier wherever possible. Wherever possible means where the conversion is straightforward and does not require modification of current api definitions.
This also involves changing the call to `deleteMessagesForTopic` appropriately. The change is done as a step to migrate to using `stream_id` instead of stream name for identifying streams. Related: zulip#3918
This is required for mark as read functionality that will be following this commit.
Also involves necessary changes at call sites, and some actionsheet functions.
Thanks for the quick revision! Merged. |
Work Done:
HeaderActionSheet
toTopicActionSheet
TopicActionSheet
TopicActionSheet
code to use stream_id instead of stream names, refactored test cases and function calls toshowTopicActionSheet
accordingly.This PR is an alternative to #4194 for marking topics as read.
For more context: https://chat.zulip.org/#narrow/stream/48-mobile/topic/mark.20as.20read
Related: #3918
Related: #3175
Fixes: #3244