Incorrect group permissions handling?

[dstillman]

https://forums.zotero.org/discussion/115481/android-different-group-permissions-than-on-desktop

[Dima-Android]

Two situations depending on what user did when user saw "You can't write to group '%s' anymore. What would you like to do?":

1. If user chose "Keep changes" then after this fix old annotations will not get synced. Only new ones. The reason is when this option is chosen we wipe the array of all pending changes in RItem hence marking all changed bits like annotations as "already synced". I may be mistaken but I think this makes it impossible for us to disect the annotations that were already synced against the ones that user really intends to sync. Definitely correct me if I am wrong.
   I mean what if there is this use-case: let's say user was part of some group that didn't allow annotations from regular users to be synced and then this group started to allow this. If we just mark all un-synced user annotations as needed to be synced and then sync them it might lead to lots of super old annotations that user never inteded to share with anyone to get synced as well.
2. User stoically ignored this dialog and didn't select any option. Then his old 'recent', annotations (the one he probably really intends to sync) will get synced OK.

Also with this fix this dialog will no longer appear for cases where the admin of the group is also the owner of the same group.

zotero-android/app/src/main/java/org/zotero/android/database/requests/MarkAllLibraryObjectChangesAsSyncedDbRequest.kt

Line 44 in b25c77a

objectS.deleteAllChanges(database = database)

zotero-android/app/src/main/java/org/zotero/android/database/objects/RItem.kt

Line 549 in b25c77a

this.changes.deleteAllFromRealm()

[dstillman]

"Keep changes" is from iOS? I'm actually not sure what that does — maybe @michalrentka or @mvasilak can comment. On the desktop, if you lose write access to a group, we show this:

You no longer have write access to the group ‘%1$S’, and changes you’ve made locally cannot be uploaded. If you continue, your copy of the group will be reset to its state on %2$S, and local changes to items and files will be lost.

If you would like a chance to copy your changes elsewhere or to request write access from a group administrator, you can skip syncing of the group now.

Or for file write access:

You no longer have file editing access for the group ‘%1$S’, and files you’ve changed locally cannot be uploaded. If you continue, all group files will be reset to their state on %2$S.

If you would like a chance to copy modified files elsewhere or to request file editing access from a group administrator, you can skip syncing of the group now.

With buttons "Reset Group [Files] and Sync" and "Skip Group".

The only options here should be to skip syncing of the group or to reset locally modified items or files to the server state. We never want to just clear pending local uploads. If we're doing that on iOS, we should definitely fix that.

[michalrentka]

"Keep changes" is from iOS? I'm actually not sure what that does — maybe @michalrentka or @mvasilak can comment. On the desktop, if you lose write access to a group, we show this:

You no longer have write access to the group ‘%1$S’, and changes you’ve made locally cannot be uploaded. If you continue, your copy of the group will be reset to its state on %2$S, and local changes to items and files will be lost.
If you would like a chance to copy your changes elsewhere or to request write access from a group administrator, you can skip syncing of the group now.

Or for file write access:

You no longer have file editing access for the group ‘%1$S’, and files you’ve changed locally cannot be uploaded. If you continue, all group files will be reset to their state on %2$S.
If you would like a chance to copy modified files elsewhere or to request file editing access from a group administrator, you can skip syncing of the group now.

With buttons "Reset Group [Files] and Sync" and "Skip Group".

The only options here should be to skip syncing of the group or to reset locally modified items or files to the server state. We never want to just clear pending local uploads. If we're doing that on iOS, we should definitely fix that.

We do have the right error message and we have options "Revert to original" and "Keep changes". "Keep changes" doesn't skip syncing of the whole group, it just marks those changes as synced and ignores them on next sync. Revert does what it should.

Anyway if we skip syncing of one group we can't re-enable it later because we don't support that on iOS yet, so that wouldn't really make sense? Maybe that's why we chose this path?

[dstillman]

"Keep changes" doesn't skip syncing of the whole group, it just marks those changes as synced and ignores them on next sync.

But that's really bad, no? It means that those changes will never be uploaded, even if you fix the permissions, and your library could be permanently out of sync (unless you edit each of the same objects to trigger reuploading).

Anyway if we skip syncing of one group we can't re-enable it later because we don't support that on iOS yet, so that wouldn't really make sense?

Don't support what, exactly? Skipping the group here really just means moving on to the next group for this sync. This message can keep appearing on the next sync. (Or it could maybe automatically skip if we hit the same error until the next manual sync or app startup, and then show again.) Or do you mean even that wouldn't work, because of some way we're handling version numbers or something on iOS?

[michalrentka]

"Keep changes" doesn't skip syncing of the whole group, it just marks those changes as synced and ignores them on next sync.

But that's really bad, no? It means that those changes will never be uploaded, even if you fix the permissions, and your library could be permanently out of sync (unless you edit each of the same objects to trigger reuploading).

Yes, those changes stay local "forever". It's been implemented like that as long as I can remember, I don't remember the reasoning behind it.

Anyway if we skip syncing of one group we can't re-enable it later because we don't support that on iOS yet, so that wouldn't really make sense?

Don't support what, exactly? Skipping the group here really just means moving on to the next group for this sync. This message can keep appearing on the next sync. (Or it could maybe automatically skip until the next manual sync or app startup.) Or do you mean even that wouldn't work, because of some way we're handling version numbers or something on iOS?

I thought you would mark the library to skip syncing until you enable it in settings again. If it just keeps popping up each sync then that wouldn't be a problem even on iOS.

[dstillman]

If it just keeps popping up each sync then that wouldn't be a problem even on iOS.

Yeah, the idea here is just that you click Skip Group and then go talk to the group admin and ask them to fix permissions and then try again. If it's not going to be fixed, you revert local changes.