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

Addressing timesheet_deleteds bug #11

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

picklecolor2
Copy link

When using the API to query timesheets_deleted, line 11 causes a problem where the query looks for timesheet_deleteds. This special cases for that class, though I'm a python amateur and there may be a cleaner way to do that.

When using the API to query timesheets_deleted, line 11 causes a problem where the query looks for timesheet_deleteds. This special cases for that class, though I'm a python amateur and there may be a cleaner way to do that.
Looks like some time recently the JSON data returned changed from a string 'true' to the value True. This will allow the wrapper to pull more than the first page of records again.
Copy link

@shree-y shree-y left a comment

Choose a reason for hiding this comment

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

Please add a test in test.py.
I used this to test your changes. Please feel free to make improvements to my test.

  # Deleted timesheets
    timesheets_deleted = None
    timesheets_deleted = api.timesheets_deleted.where(modified_since=datetime(2015,7,1), user_ids=[current_user.id]).all()
    if len(timesheets_deleted) > 0:
        print (' - Found {0} deleted timesheets'.format(len(timesheets_deleted)))
    else:
        print(' - No deleted timesheets found')
        return

@@ -9,6 +9,8 @@ def __init__(self, url, options, repo, bridge, is_singleton=False, mode="list"):
self.repo = repo
self.model = repo.model
self.name = class_to_endpoint(self.model.__name__) + ('' if is_singleton else 's')
if isinstance(self.repo, repos.TimesheetsDeleted):
self.name = 'timesheets_deleted'
Copy link

Choose a reason for hiding this comment

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

What you have here works, but wondering if it is better to create a method in repository.py to set the name.
We can avoid one-off logic like this, and also if new endpoints are added that follow timesheets_deleted pattern, then we wouldn't need to update the logic here again.
Thoughts?

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

Successfully merging this pull request may close these issues.

2 participants