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

Distinguish user status and machine status #230

Merged
merged 7 commits into from
Dec 10, 2023
Merged

Conversation

NinaWie
Copy link
Collaborator

@NinaWie NinaWie commented Dec 7, 2023

This is how the proposed solution works:

  1. We use the attribute active in the json files to encode the machine status. This can be either a bool (true / false) or a string (active / out-of-order / retired). Since we haven't used the active attribute in the frontend so far, we can change it without loosing backward compatability.
  2. The app does not read the status attribute of the json file anymore. We have to keep it in the json files for a while though, since removing it will make the app crash for all users with an old version of the app.
  3. When the data is loaded, the machine status is set dependent on the json file entry, while the user status is always unvisited except if the machine is retired or out of order.
  4. After loading the initial data, the json file that is saved locally on the user's phone is loaded, which overwrites the user status (to visited / marked). This has the following drawback: If the machine is retired, but the user selects "visited" by mistake, there is no way of going back (because there is no possibility for the user to select "retired"). I think we have to accept this drawback, because on the other hand we don't want to overwrite the status if the user has actually visited the machine before it was retired.

@jannisborn jannisborn added the enhancement New feature or request label Dec 8, 2023
Copy link
Owner

@jannisborn jannisborn left a comment

Choose a reason for hiding this comment

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

It's a super super useful feature, really love this effort! Good idea also to display a message when the status button is tapped.

I think we have to take care of some things to avoid a mess:

  • If we change the type of the "active" attribute, we should do this consistently within all/server-locations in the same PR. Maybe we actually just remove the attribute and add a new one called "machine_status" with option as described here. For now we dont use "out-of-order" but we will populate this over time.
  • Regarding 1) We can already change this attribute in all/server locations and once users download the new version they will have the new features. Users with the old version dont have the feature, but I dont see a reason why this should create issues with backwards compatibility (server-locations will also be reloaded!)
  • Regarding 2) Good catch, agreed!
  • Regarding 3) I disagree that this is a good strategy, see my comment in the code. The user status can still be visited or marked even if machine status is retired/OOO
  • Regarding 4) I dont see your problem because the user status will always default to "unvisited". It cant be "retired" (as per my other comment), so this problem does not exist. If the machine is retired and the user selects "visited" the pin will be green, if the user then un-visits the machine and selects "unvisited" again, we just have to make sure to display it in gray and not in red (as I had described in my original comment)

PennyMe/PinViewController.swift Outdated Show resolved Hide resolved
PennyMe/Artwork.swift Outdated Show resolved Hide resolved
@NinaWie
Copy link
Collaborator Author

NinaWie commented Dec 8, 2023

I will make the other changes regarding naming etc later, and agree to your points, but just about this: What you're suggesting is that we color all the pins according to the status attribute, except for the pins with status = unvisited and machine_status=retired (or out of order), which will be grey? That should work fine without the problem I described
I didn't see your comment in the other PR, but makes sense now.

Regarding the renaming of all attributes in all/server_locations, from my side it's not necessary - I don't get your comment We can already change this attribute in all/server locations and once users download the new version they will have the new features - even if we don't change this attribute, the users will still have the new feature, because it also works with the boolean active attribute.

@jannisborn
Copy link
Owner

Ok Great!

Regarding the change in the JSON files, sorry, I get it now. I'll include the changes in one of the next machine-update PRs. Because also the location differ will have to be adapted slightly (I'm taking care)

@NinaWie
Copy link
Collaborator Author

NinaWie commented Dec 9, 2023

I adapted the field names and in order to test it, I now already changed the all_locations and server_locations. I removed the active attribute in both of them, added a machine_status attribute in both of them, and removed the status attribute in the all_locations.json. If you want me to revert it because of merge conflicts with the new machine updates, let me know.

The colouring of the pins and everything works fine now in my tests

Copy link
Owner

@jannisborn jannisborn left a comment

Choose a reason for hiding this comment

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

Very good I think we're almost done just:

  • there's a merge conflict now (we can stall the other open PR Backend refactors #232 behind this) but we still need to resolve the conflict, it probably affects the machines changed in Updates from website (server_locations) #231
  • The updated json files have lots of machines where "machine_status" is retired but "status" is also retired. I thought we leave this to "unvisited" because it denotes the personal collection status. (At this point, we could even consider removing this attribute entirely, right?)

@NinaWie
Copy link
Collaborator Author

NinaWie commented Dec 10, 2023

Regardign the merge conflict, I would suggest to undo my changes in the server_locations.json, then merge the branch, and then apply the changes again (adding machine_status attribute and deleting active). Regarding the retired attribute: We for sure need the status=retired because otherwise everyone with the older app version won't see that a machine is retired. So I would not remove the attribute at the moment.

@NinaWie NinaWie merged commit 25f4795 into main Dec 10, 2023
1 check passed
@jannisborn jannisborn deleted the retired_fields_change branch December 10, 2023 21:18
@jannisborn jannisborn linked an issue Jan 14, 2024 that may be closed by this pull request
@jannisborn jannisborn mentioned this pull request Jan 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Status toggles
2 participants