Rdsimaps3180 fix unit test carto user

[acarrera42]

I would check what are the system timezone for carto machines. Madrid is UTC +1. The issue we ran into primarily occurs if you are behind UTC such as NY EST which is UTC - 5 right now.

[gfiorav]

Sorry, I wasn't clear:

Why add a new method? Seems a lot of change for appending .utc. That's all :)

[acarrera42]

My understanding (which could be incorrect) is ActiveRecord automatically adds a getter method for the columns in the db model. Therefore I needed a way to override this method to insure UTC timezone was always added. I'm open to more elegant solutions if ruby/ActiveRecord allows 😄

[gfiorav]

Oh, I didn't realize that's what you're doing. Override attribute setter/getters like this is risky stuff!

In any case, try:

def overridden_method
  super.utc
end

That might do what you're looking for, but you are loosing reference to the original object (which is why it's not a good idea). This has caused headaches in the past. Also, you now need to take care of trasnforming nil objects and other stuff (which AR does for you when you don't override).

I'd research an AR-approved way of doing this without overriding

[acarrera42]

Per conversation with carto. We are marking all billing unit tests pending for Bloomberg since we do not use the billing logic in production at the moment.

https://github.com/CartoDB/cartodb/issues/13371 (with a PR) has been submitted to cartodb to review and add to upstream.

[gfiorav]

I thought you were going to explicitly set the timezone in the specs (to make the tests more robust), PR those tests changes to CARTO and also raise awareness of billing timezone-dependence.

How come you're setting the tests pending?

[acarrera42]

Perhaps I misunderstood. I thought the Bloomberg code would be pending and the carto PR would set the timezone....

[gfiorav]

Just to express it in a it more detail so we can find where we're diverging:

This is yet another public service that Bloomberg is doing to CARTO, since once again you found that tests make an assumption on their execution conditions that is not explicitly enforced (which is an error; all environment conditions needed for the test should be explicitly set). On top of that, you've also raised the issue that billing won't properly work in other timezones.

Therefore, I think the actions should be as follows: 1) Enforce the timezone conditions for tests to pass, and PR those changes to CARTO. 2) Raise awareness of the billing system's fragility via an Issue in CARTO's repo.

Doing this, we get the following:

• Passing tests for the timezone-specific billing system (which you don't use but that's beside the fact)
• A possible fix to the billing system from CARTO (in due time)
• A contribution to CARTO's unit tests

I'm sorry if I wasn't clear enough in my original comment! :)

[acarrera42]

Regarding the action items for submitting the changes to carto:

1. https://github.com/CartoDB/cartodb/pull/13372/files
2. https://github.com/CartoDB/cartodb/issues/13371

In term of modifying Bloomberg's code base there are two options available:

1. Mark the billing unit tests as pending until Bloomberg uses the logic (and hopefully carto upstream address the above)
2. Apply the above PRs to Bloomberg's code
3. Force the rspecs to set the timezone in Bloomberg's code.

Of the above the first or second seems most cost effective. However the second option adds additional unnecessary risk at this particular time and should be rolled out after the merge code is released. The third option is not as straight forward to do as it would initially seem.

[gfiorav]

I'll make one more push for not modifying the code at all and mark test pending with appropriate messages and reasons. After that, I can disagree and commit to whatever you think is best! My argument is:

Since you're not using it, we can save modifying upstream code.

[acarrera42]

Pushed update to pending comments to include a link to the GitHub issue in upstream carto.