Feature/CLOUD-396 #76
Conversation
dugajean
left a comment
dugajean
left a comment
There was a problem hiding this comment.
Looking good! Awesome work.
| if ( isset( $usage['credits'] ) ) { | ||
| // New credit system. | ||
| $usage['credits'] = array( | ||
| 'limit' => $usage['credits']['limit'], |
There was a problem hiding this comment.
@DavidCramer I notice that we are checking if the credits key is set before we run this logic, but I wonder if that's enough. My concerns here are what about all these child keys, that might not be set. To add to it, it seems that we are getting this information from a source that we don't control. Ideally, this should be addressed on usage_stats() — setting some defaults —, so we can be relaxed using the logic elsewhere. Thoughts?
There was a problem hiding this comment.
@pereirinha - That is a good suggestion. However, the issue is that there are two different style of settings, although we could switch between the defaults.
Also, this is directly from their API, and is documented. So it shouldn't really change. If it did, we would have it break without breaking.
An ideal way, would be to check when calling the API, then output an error before actually setting the value.
I'll let this pass for now, then loop back with a clean up ticket.
pereirinha
left a comment
pereirinha
left a comment
There was a problem hiding this comment.
There are a few merge conflicts that need attention, but once resolved, it can be merged.
Great work.
|
@pereirinha - I altered it a little by adding in a method to fetch a specific stat. This will ensure we never reference something that doesn't exist. |
pereirinha
left a comment
pereirinha
left a comment
There was a problem hiding this comment.
Thanks for the update @DavidCramer. This update looks more robust solution.
I think I found a problem and I left you a comment.
Can you confirm that there's one?
cloudinary-image-management-and-manipulation-in-the-cloud-cdn/php/class-connect.php
Show resolved
Hide resolved
pereirinha
left a comment
pereirinha
left a comment
There was a problem hiding this comment.
Great work. This looks awesome. I was able to test this by tweaking the transient information on transformations and it worked beautifully.
Alert/Admin notices shown at 70%, 80%, and 90% (with a link to upgrade at 90%)
Dashboard Settings page added usage summary.
