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.

Sign up for GitHub

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

Jump to bottom

Add support for all types of monthly repeating schedules #1462

Merged
merged 4 commits into from
Sep 17, 2024
Merged

Add support for all types of monthly repeating schedules #1462

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
42754ac
Fix debug logging of API responses and add a small warning
bcantoni Sep 13, 2024
98078b2
Add failing test for "monthly on first Monday" schedule
bcantoni Sep 13, 2024
9c6cc8f
Add support for all monthly schedule variations
bcantoni Sep 13, 2024
4dd4e48
Simplify validation check and correctly handle int vs string values
bcantoni Sep 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 28 additions & 14 deletions tableauserverclient/models/interval_item.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,21 +246,35 @@ def interval(self):

@interval.setter
def interval(self, interval_values):
# This is weird because the value could be a str or an int
# The only valid str is 'LastDay' so we check that first. If that's not it
# try to convert it to an int, if that fails because it's an incorrect string
# like 'badstring' we catch and re-raise. Otherwise we convert to int and check
# that it's in range 1-31
# Valid monthly intervals strings can contain any of the following
# day numbers (1-31)
# relative day within the month (First, Second, ... Last)
# week days (Sunday, Monday, ... LastDay)
VALID_INTERVALS = [
"Sunday",
"Monday",
"Tuesday",
"Wednesday",
"Thursday",
"Friday",
"Saturday",
"LastDay",
"First",
"Second",
"Third",
"Fourth",
"Fifth",
"Last",
]
for interval_value in interval_values:
error = "Invalid interval value for a monthly frequency: {}.".format(interval_value)

if interval_value != "LastDay":
try:
if not (1 <= int(interval_value) <= 31):
raise ValueError(error)
except ValueError:
if interval_value != "LastDay":
raise ValueError(error)
try:
if not (1 <= int(interval_value) <= 31):
error = f"Invalid monthly numeric frequency interval: {interval_value}."
Copy link

@anyoung-tableau anyoung-tableau Sep 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This error message can never happen right? Its ValueError will get caught below and either swallowed or a new ValueError will be thrown. I haven't used Python much, is it common to use exceptions to control flow?

Maybe use the isinstance(interval_value, (int, float)) construct like on line 80 to detect if it's a number first.

Or if it's always a string, interval_value.isdigit()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well in theory I think it would fail right there if the numerical value was 32 or "32".

I agree using exceptions here is a little funky. I was trying to fit in with the methods used already.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using exceptions for control flow isn't uncommon in python. The except ValueError on 274 would indeed catch and consume the one raised on 273. The type check proposed by @anyoung-tableau does seem like a good alternative to let the different errors surface.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll take another look at it. The challenge is that even numbers are strings by the time they land here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Easiest way to do it seems like explicitly listing the known allowed digits:
for value in range(1, 32):
VALID_INTERVALS.add(str(value))

raise ValueError(error)
except ValueError:
if interval_value not in VALID_INTERVALS:
error = f"Invalid monthly string frequency interval: {interval_value}."
raise ValueError(error)

self._interval = interval_values

Expand Down
4 changes: 3 additions & 1 deletion tableauserverclient/server/endpoint/endpoint.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,9 @@ def _make_request(

loggable_response = self.log_response_safely(server_response)
logger.debug("Server response from {0}".format(url))
# logger.debug("\n\t{1}".format(loggable_response))
# uncomment the following to log full responses in debug mode
# BE CAREFUL WHEN SHARING THESE RESULTS - MAY CONTAIN YOUR SENSITIVE DATA
# logger.debug(loggable_response)

if content_type == "application/xml":
self.parent_srv._namespace.detect(server_response.content)
Expand Down
12 changes: 12 additions & 0 deletions test/assets/schedule_get_monthly_id_2.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
<?xml version='1.0' encoding='UTF-8'?>
<tsResponse
xmlns="http://tableau.com/api"
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://tableau.com/api https://help.tableau.com/samples/en-us/rest_api/ts-api_3_25.xsd">
<schedule id="8c5caf33-6223-4724-83c3-ccdc1e730a07" name="Monthly First Monday!" state="Active" priority="50" createdAt="2024-09-12T01:22:07Z" updatedAt="2024-09-12T01:22:48Z" type="Extract" frequency="Monthly" nextRunAt="2024-10-07T08:00:00Z" executionOrder="Parallel">
<frequencyDetails start="08:00:00">
<intervals>
<interval weekDay="Monday" monthDay="First" />
</intervals>
</frequencyDetails>
</schedule>
</tsResponse>
16 changes: 16 additions & 0 deletions test/test_schedule.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
GET_HOURLY_ID_XML = os.path.join(TEST_ASSET_DIR, "schedule_get_hourly_id.xml")
GET_DAILY_ID_XML = os.path.join(TEST_ASSET_DIR, "schedule_get_daily_id.xml")
GET_MONTHLY_ID_XML = os.path.join(TEST_ASSET_DIR, "schedule_get_monthly_id.xml")
GET_MONTHLY_ID_2_XML = os.path.join(TEST_ASSET_DIR, "schedule_get_monthly_id_2.xml")
GET_EMPTY_XML = os.path.join(TEST_ASSET_DIR, "schedule_get_empty.xml")
CREATE_HOURLY_XML = os.path.join(TEST_ASSET_DIR, "schedule_create_hourly.xml")
CREATE_DAILY_XML = os.path.join(TEST_ASSET_DIR, "schedule_create_daily.xml")
Expand Down Expand Up @@ -158,6 +159,21 @@ def test_get_monthly_by_id(self) -> None:
self.assertEqual("Active", schedule.state)
self.assertEqual(("1", "2"), schedule.interval_item.interval)

def test_get_monthly_by_id_2(self) -> None:
self.server.version = "3.15"
with open(GET_MONTHLY_ID_2_XML, "rb") as f:
response_xml = f.read().decode("utf-8")
with requests_mock.mock() as m:
schedule_id = "8c5caf33-6223-4724-83c3-ccdc1e730a07"
baseurl = "{}/schedules/{}".format(self.server.baseurl, schedule_id)
m.get(baseurl, text=response_xml)
schedule = self.server.schedules.get_by_id(schedule_id)
self.assertIsNotNone(schedule)
self.assertEqual(schedule_id, schedule.id)
self.assertEqual("Monthly First Monday!", schedule.name)
self.assertEqual("Active", schedule.state)
self.assertEqual(("Monday", "First"), schedule.interval_item.interval)

def test_delete(self) -> None:
with requests_mock.mock() as m:
m.delete(self.baseurl + "/c9cff7f9-309c-4361-99ff-d4ba8c9f5467", status_code=204)
Expand Down
Loading