settings_components: Split functions to improve type checking. #30090
Conversation
web/src/settings_org.js
Outdated
| property_value = settings_components.get_stream_settings_property_value(property_name, sub); | ||
| } else { | ||
| property_value = settings_components.get_realm_settings_property_value(property_name); | ||
| } |
There was a problem hiding this comment.
We should also split this function, like the others it's mainly a bunch of switch statements for individual settings and thus will do nicely being split up.
afeefuddin
force-pushed
the
settings_components_refactor
branch
from
006a7d6
to
bb83d80
Compare
afeefuddin
force-pushed
the
settings_components_refactor
branch
3 times, most recently
from
3a6790c
to
96b4987
Compare
afeefuddin
force-pushed
the
settings_components_refactor
branch
from
96b4987
to
49cce92
Compare
|
Ready for review! Naming might be a concern because the names of the functions aren't very concise. |
web/src/settings_org.js
Outdated
| update_dependent_subsettings(property_name); | ||
| } | ||
|
|
||
| export function discard_stream_proprty_element_changes(elem, sub) { |
There was a problem hiding this comment.
property has a typo here.
|
I think this version does achieve our type-checking goals; @sahil839 should probably do a round of review and testing to make sure it doesn't regress anything. @afeefuddin how have you tested this? |
|
@afeefuddin can you also rebase this? Some reafactoring changes in the settings code have been merged recently. |
afeefuddin
force-pushed
the
settings_components_refactor
branch
from
49cce92
to
2b19496
Compare
|
Done with rebasing @sahil839. One test is cancelled for some reason. I have tested a few functions manually, but I don't know how I should test this otherwise. |
| if (property_name === "can_mention_group") { | ||
| proposed_val = get_dropdown_list_widget_setting_value($elem); | ||
| } else if (current_val !== undefined) { | ||
| proposed_val = get_input_element_value(elem, typeof current_val); |
There was a problem hiding this comment.
Can we update this to use switch like other functions do? We would soon add new settings so would be good to just write the code considering that.
| if (property_name === "can_mention_group") { | ||
| data[property_name] = JSON.stringify({ | ||
| new: input_value, | ||
| old: group!.can_mention_group, | ||
| old: group.can_mention_group, |
There was a problem hiding this comment.
This code will only be called for can_mention_group and we expect all future group settings to follow the same format, so maybe we can just remove the if condition and call this directly.
And then if we have some other fields which are not group settings using the save discard widget, then we can update this later.
|
A good way of testing would to test all the sections where save-discard widget is used by verifying that the setting is changed, setting is discarded on clicking "Discard" and also by resetting the setting by selecting the same value after changing it once and check that the save-discard widget disappears as expected. |
|
Also, would it be worth moving functions for streams and groups to respective files like |
Refactor get_property_value and check_property_changed to improve type checking and code readability.
Self-review checklist
(variable names, code reuse, readability, etc.).
Communicate decisions, questions, and potential concerns.
Individual commits are ready for review (see commit discipline).
Completed manual review and testing of the following: