Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Execute the user_settings object transition #4933

Closed
timabbott opened this issue Aug 1, 2021 · 2 comments · Fixed by #5379
Closed

Execute the user_settings object transition #4933

timabbott opened this issue Aug 1, 2021 · 2 comments · Fixed by #5379
Labels
api migrations P1 high-priority

Comments

@timabbott
Copy link
Member

timabbott commented Aug 1, 2021 •
edited by gnprice
Loading

In zulip/zulip#19404, we just added the feature level 89 migration to change how we encode user settings in the Zulip API to use a user_settings object.

The API documentation should explain the transition clearly. Here's the recommended strategy:

  • Clients can immediately switch from processing the update_display_settings and update_global_notifications event types and process the user_settings event type instead.
  • Clients may then want to refactor how they store user settings to have a common module/object.
    The web app did this migration in zulip/zulip@998d710.
  • Clients can then pass the user_settings_object client capability to all Zulip server versions (older servers will ignore it). If the server returns a user_settings object in the register response, the client should use that to initialize its user_settings object using the user_settings portion of the response; otherwise it should use the user personal settings flags that appear in the top-level response.

I've tentatively tagged this as a priority, because there is a decent amount of messy complexity involved in maintaining the legacy API here, and we'd prefer to be able to remove that complexity.
@chrisbobbe
Copy link
Contributor

chrisbobbe commented Aug 2, 2021 •
edited
Loading

@gnprice it would be great to get your thoughts on #4659 (comment) before we proceed with this. Actually, I think this is unnecessary to complete this.

@chrisbobbe chrisbobbe mentioned this issue Mar 15, 2022
Open
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Mar 15, 2022
@chrisbobbe
initial data types: Add TODOs to update to modern user_settings object
63e79e0 
And mark write-only (with the contravariance sigil, `-`) the
deprecated properties that appear in our types but that we're not
currently using, so that we won't accidentally start using them.

This should be all of them. I checked all 36 results of the search
term [did not include user_settings_object] on
https://zulip.com/api/register-queue on 2022-03-15. Several of the
36 were properties that we don't use and that don't appear in our
types at all. (No use adding those, I think, just for them to be
removed when we do the migration.)

Related: zulip#4933
@chrisbobbe chrisbobbe mentioned this issue Mar 15, 2022
Merged
@chrisbobbe
Copy link
Contributor

Just sent #5299 as a first step for this.

chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue May 11, 2022
@chrisbobbe
initial data types [nfc]: Put enter_sends under update_display_settings
41de234 
(And remove the TODO comment, because there's already one on the
whole of InitialDataUpdateDisplaySettings.)

This is where it belongs. Oddly, the doc lists this property among
many that come with realm_user, which I guess is why it ended up in
InitialDataRealmUser.. But the doc still says "Present if
`update_display_settings` is present in `fetch_event_types` […]" for
this one:
  https://zulip.com/api/register-queue
, which is what we'd expect, given that this is one of the values
that's migrated to the new `user_settings` object; see zulip#4933.
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue May 11, 2022
@chrisbobbe
initial data: Add 'user_settings' to fetch_event_types; update Flow t…
e91dd05 
…ypes

Related: zulip#4933
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue May 11, 2022
@chrisbobbe
api: Execute the user_settings object transition
da2f477 
In this commit:

- Use the `user_settings` initial-data object, and corresponding
  event type, new in 5.0 (FL 89). We added 'user_settings' to
  `fetch_event_types` in the previous commit; that's how we're
  opting in to the new shape from servers that offer it.

- Declare the `user_settings_object` client capability. This means
  nothing to pre-5.0 servers, which will ignore the capability and
  continue to send the data in the legacy way. But it makes newer
  servers drop the legacy behavior, which helps clear the way for
  server maintainers to eventually delete the legacy code.

Fixes: zulip#4933
@chrisbobbe chrisbobbe mentioned this issue May 11, 2022
Merged
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue May 13, 2022
@chrisbobbe
initial data: Add 'user_settings' to fetch_event_types; update Flow t…
d738db5 
…ypes

Related: zulip#4933
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue May 13, 2022
@chrisbobbe
api: Execute the user_settings object transition
0860224 
In this commit:

- Use the `user_settings` initial-data object, and corresponding
  event type, new in 5.0 (FL 89). We added 'user_settings' to
  `fetch_event_types` in the previous commit; that's how we're
  opting in to the new shape from servers that offer it.

- Declare the `user_settings_object` client capability. This means
  nothing to pre-5.0 servers, which will ignore the capability and
  continue to send the data in the legacy way. But it makes newer
  servers drop the legacy behavior, which helps clear the way for
  server maintainers to eventually delete the legacy code.

Fixes: zulip#4933
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue May 13, 2022
@chrisbobbe
initial data types [nfc]: Put enter_sends under update_display_settings
6d01851 
(And remove the TODO comment, because there's already one on the
whole of InitialDataUpdateDisplaySettings.)

This is where it belongs. Oddly, the doc lists this property among
many that come with realm_user, which I guess is why it ended up in
InitialDataRealmUser.. But the doc still says "Present if
`update_display_settings` is present in `fetch_event_types` […]" for
this one:
  https://zulip.com/api/register-queue
, which is what we'd expect, given that this is one of the values
that's migrated to the new `user_settings` object; see zulip#4933.
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue May 13, 2022
@chrisbobbe
initial data: Add 'user_settings' to fetch_event_types; update Flow t…
7de5144 
…ypes

Related: zulip#4933
This was referenced May 20, 2022
Open
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
api migrations P1 high-priority
Projects
None yet
Milestone
No milestone
2 participants
@timabbott @chrisbobbe