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.

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

settings shows user's timezone, provide ptr to fetch user #594

Conversation

krugerk
Copy link
Contributor

@krugerk krugerk commented Dec 31, 2024

Summary

Settings shows the user's timezone but only a few places in the app were fetching the user: fetching default notification settings (entering the default notification settings screen) and upon signing in.
This MR allows the user to trigger a fetch of the User endpoint via a Pull-To-Refresh on the Settings screen.

For UI changes including screenshots of before and after is great.
settings - ptr

Validation

used the app in the simulator
change the timezone at the website
tapped around in the app again both entering settings and pull to refresh from settings

Fixes #507 and helps out with #360

@krugerk krugerk requested a review from a team as a code owner December 31, 2024 12:08
@krugerk krugerk requested review from theospears and removed request for a team December 31, 2024 12:08
@theospears
Copy link
Collaborator

I have a PR in the works which will update User every time goals are updated (as a side effect of moving to use diff_since and incremental goal update). If that change is merged, would you still want to do this?

@krugerk
Copy link
Contributor Author

krugerk commented Dec 31, 2024

I have a PR in the works which will update User every time goals are updated (as a side effect of moving to use diff_since and incremental goal update). If that change is merged, would you still want to do this?

It is a very basic implementation as is. Of course if user is being fetched more often anyway, then one like this at the settings screen is perhaps too basic.
I think it is fine like this for now and maybe yours with using diff_since makes it moot.

@@ -87,6 +90,19 @@ class SettingsViewController: UIViewController {
func showLogsTapped() {
self.navigationController?.pushViewController(LogsViewController(), animated: true)
}

@objc private func fetchUser() {
Task { [weak self] in
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's some interesting nuance to lifetimes here.

It might be tempting to do this:

          guard let self = self else { return }

But it's not quite right, as the view might be dismissed while waiting for the refreshUser call to complete, and in that case we don't want to hold on to self.

Maybe worth a comment making this clear?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as in,

@objc private func fetchUser() {
        Task { [weak self] in
            self?.tableView.isUserInteractionEnabled = false

            try? await ServiceLocator.currentUserManager.refreshUser()

            guard let self else { return }
            self.tableView.reloadData()
            self.tableView.layoutIfNeeded()
            self.tableView.refreshControl?.endRefreshing()
            
            self.tableView.isUserInteractionEnabled = true
        }
    }

@theospears theospears merged commit 741432b into beeminder:master Jan 1, 2025
1 check passed
@krugerk krugerk deleted the bugfix/507-app-not-noticing-when-user-changed-accounts-timezone branch January 3, 2025 12:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

App doesn't notice when you switch Beeminder timezone
2 participants