Allow to identify the specific DownloadBatchRequirementRule that was violated, causing the download to fail

Konrad-Morawski commented 4 years ago

The problem

As signalled in https://github.com/novoda/download-manager/issues/525

While the library currently allows for setting up multiple requirement rules (DownloadBatchRequirementRule) that have to be met in order for the download to be successful, there is no way for determining which of the rules was violated, once it happens. The DownloadError instance that the calling code gets presented with only contains a general indication that some rule was violated (REQUIREMENT_RULE_VIOLATED). This poses a problem in scenario where the client code needs to set multiple rules, and its reaction to an error should differ depending on which rule was violated.

Real life example

Client app determines if a requested download can be completed successfully based on whether there's enough storage on the device (rule 1). It also allows the user to set an arbitrary cap on the amount of storage made available for downloads - a value customizable in the app settings. This requirement will be encoded as rule 2. Once a download can't come through, the expected reaction will differ.

If there's no available storage (rule 1), the user is presented with an error.
If the available storage is below the currently set limit (rule 2), the user is prompted to increase the limit in the settings.
The solution

DownloadBatchRequirementRule provides a numeric code, identifying the rule. Once a rule violation is detected, the error object (DownloadError) will contain the numeric value, allowing the client code to determine the appropriate course of action for the specific rule.

Optionality of the feature

The code is implemented as a nullable Integer value. Returning null from DownloadBatchRequirementRule#getCode is allowed, and effectively disregards the feature.

hal9002 commented 4 years ago

Can one of the admins review this PR?

Konrad-Morawski commented 4 years ago

Rebased and conflicts resolved ;)

zegnus commented 4 years ago

test this please

Konrad-Morawski commented 4 years ago

I'll get back to it as soon as I can.

blundell commented 4 years ago

@Konrad-Morawski What's the idea of the Integer code being nullable?

Konrad-Morawski commented 4 years ago

@blundell if you don't need this functionality, just don't use it, by returning null instead of a code. We don't want to force client apps to make up some irrelevant numeric value; null clearly indicates an absence of one. Or in other words, when you return null, everything works just the same way as before this PR.

blundell commented 4 years ago

@Konrad-Morawski but you added a code to the demo app's rule, and don't make use of it. Encouraging an "irrelevent numeric value" + possibility to be null.

Trying to get my head around the API, as the code diff also includes the change so that the composite rule DownloadBatchRequirementRules is now part of the public API. I was trying to comprehend the change.

Konrad-Morawski commented 4 years ago

@Konrad-Morawski but you added a code to the demo app's rule, and don't make use of it. Encouraging an "irrelevent numeric value" + possibility to be null.

Good point. I guess it should be left as null.

An alternative would be to actually provide a full-blown demo of this functionality, but given that it's quite specific / advanced use case, demo-simple probably isn't actually a good place for that. I can just make it null instead.

Trying to get my head around the API, as the code diff also includes the change so that the composite rule DownloadBatchRequirementRules is now part of the public API.

Good catch again. I've inspected it and it looks redundant to me, there should be no reason to implement this interface in the client code. I'll drop the public.

jakubvimn commented 4 years ago

Is there anything else that should be done before merging this change? @zegnus @Mecharyry when could you make the code review?

zegnus commented 4 years ago

@Konrad-Morawski I've made a few comments, have a look when you have some minutes.

Konrad-Morawski commented 3 years ago

I know it's been a year, I apologize for the glacial speed of addressing the remarks. Literally a long story.

Anyhow, I've updated the PR as requested : ) I'd be happy if you took another look, whether any time soon or - following the pace I myself set - around mid-2022.

Thanks @zegnus

novoda / download-manager