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

Background service for polling the server #1700

Closed
wants to merge 1 commit into from

Conversation

grzesiek2010
Copy link
Member

Closes #1668

What has been done to verify that this works as intended?

I've confirmed that a notification is displayed if there is a form update available. Additionally, we can click on such a notification and we are moved to FormDownloadList.

Why is this the best possible solution? Were any other approaches considered?

I investigate how to implement background service and found out that there are a few different solutions and some of them work well on specific os versions.
I found this powerful library https://github.com/evernote/android-job

Depending on the Android version either the JobScheduler, GcmNetworkManager or AlarmManager is getting used.

So it's the best solution in our case and it's really easy to use.

Are there any risks to merging this code? If so, what are they?

I can't see anything risky here.

Do we need any specific form for testing your changes? If so, please attach one.

No.

25344604_1649365898460242_503585298_o

@grzesiek2010
Copy link
Member Author

@lognaturel please take a look and let us know if we can start testing or maybe you want to change something.

@lognaturel
Copy link
Member

lognaturel commented Dec 12, 2017

Thanks for working hard on this! I know this is a feature you're eager to get in.

One small thing I see of the bat is that the setting seems like a user-level setting rather than an admin-level one to me. Probably under form management.

I can't see anything risky here.

Risk evaluation is all about thinking about what has changed and what the worst case side-effects are. I see several possible categories of risk that need to be considered and addressed:

  • Risks associated with adding a dependency. Is it maintained? Does it change the binary size? Is it likely to change dramatically which would lead to increased maintenance burden?
  • Risks associated with moving a lot of code around. This could introduce bugs into the recently-added form version checking. There's no revision history so it's really hard to tell what just moved, what is entirely new, etc.
  • Risks associated with things that are disruptive to users. Does this keep track of forms that have already resulted in notifications and make sure the user is never notified about that form again? Could this accidentally result in unwanted notifications?
  • Risks associated with changing settings. Could any other settings have been accidentally affected?

Would be great to get your thoughts on some of those. If anyone has a chance to do some verification, that would be great! My tendency would be not to rush it to the upcoming release but if we can adequately address all the risks then it would be reasonable.

@grzesiek2010
Copy link
Member Author

One small thing I see of the bat is that the setting seems like a user-level setting rather than an admin-level one to me.

really? If you are sure I can move it there but it's not so clear to me.

Does it change the binary size?

8,7mb [master_branch]
8,8mb [this branch]

Risks associated with moving a lot of code around.

it's always risky but I just copied the code. the code is not changed.

Risks associated with changing settings. Could any other settings have been accidentally affected?

No, I can't see such a case but settings should be tested (saving/reading new settings) when we add new settings.

My tendency would be not to rush it to the upcoming release

I know this is a feature you're eager to get in

No problem we can add this feature in v1.13.0

@getodk-bot
Copy link
Member

Heads up @grzesiek2010, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/master branch and resolve your pull request's merge conflicts accordingly.

@grzesiek2010
Copy link
Member Author

@yanokwa @lognaturel

I was thinking about this a little bit today especially about the issue you have pointed out - notification is shown multiple time for the same forms if we ignore that and don't download a newer version.

Probably we can just add a new column in forms.db like newerVersionDetected or something like that and after showing a notification I can mark forms we should ignore in future.

Please let me know if I should go ahead.

@yanokwa
Copy link
Member

yanokwa commented Feb 6, 2018

I talked to @lognaturel a bit about this and I think we should put this on pause until we get consensus from the TSC on how to do notifications.

@grzesiek2010
Copy link
Member Author

Ok, let me know when you have some new information about it.

@lognaturel
Copy link
Member

Closing for now since I think it will take a bit to get back to. Please keep the branch, @grzesiek2010.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants