This is a prep commit such that we can avoid duplicate code when we
unsubscribe bots for inaccessible private streams when changing owner
or reactivating them.
I found the previous model for computing what settings to use for
streams increasingly difficult to understand, which is generally a
recipe for future bugs.
Refactor to have a clear computation of what complete permissions
state the client is requesting, validate that state, and then pass
that state to the do_change_stream_permission.
We now allow changing access to history of the stream by only passing
"history_public_to_subscribers" parameter. Previously, "is_private"
parameter was also required to change history_public_to_subscribers
otherwise the request was silently ignored.
We also raise error when only history_public_to_subscribers parameter
is passed with value False without "is_private: True" for a public
or web-public stream since we do not allow public streams with
protected history.
This commit removes the unnecessary assertion statements in
do_change_stream_permission for case when "is_web_public" is
True, since we already check those cases in the view function
update_stream_backend and this is the only place from where
do_change_stream_permission is called.
We aim to remove other assertions also from there as mentioned
in the comment and instead check the values in caller itself.
This commit adds code to send stream creation and peer add events
when stream is changed from private to public. These events are
only sent to users who are not susbcribed to the stream and are
not realm admins as subscribers and realm admins already have
the stream data. This will update the stream data with clients
and will remove the need to reload to view the modified stream.
Fixes#22194.
This commit removes "role" field from subscription
objects since we are not moving forward with stream
administrator concept and instead working on new
permssions model as per #19525.
As discussed in the new comments results in a better failure mode if
an error occurs while adding subscriptions; running the merge tool
again after fixing whatever caused the error will work just fine.
While it is possible to have `stream.recipient_id` being `None`,
the code works under the assumption that it is not. Potentially
we will get a runtime error, but it is not quite explicit without
the assertion.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
Add none-checks, rename variables (to avoid redefinition of
the same variable with different types error), add necessary
type annotations.
This is a part of #18777.
Signed-off-by: Zixuan James Li <359101898@qq.com>
We remove one call to get_occupied_streams to get occupied
streams before unsubscribing because we already know which
streams can become vacant, i.e. the one from which users are
being unsubscribed, and we can directly use the list of streams
from which users are being unsubscribed and get vacant streams
by checking which of these streams are not in get_occupied_streams
called after unsubscribing users.
Previously, we were marking messages of all the streams passed
to bulk_remove_subscriptions even if user was not subscribed
to some of them and those streams would ideally not have
any unread messages. This code was added in 766511e519.
This commit changes the code to only mark messages of actually
unsubscribed streams as read.
Sahil Batra
Zixuan James Li
Tim Abbott
Adam Sah
Zixuan James Li
Anders Kaseorg