-
Notifications
You must be signed in to change notification settings - Fork 74
[#120] method to list tickets #800
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
base: main
Are you sure you want to change the base?
Changes from all commits
fec88de
b5c2033
5b0873a
3323b9d
5626233
b133459
2d23c0f
e456d01
adea12d
66f50c4
9d728cb
93c5bcb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,20 +1,25 @@ | ||
| #! /usr/bin/env python | ||
|
|
||
| import calendar | ||
| import datetime | ||
| import logging | ||
| import os | ||
| import sys | ||
| import unittest | ||
| import time | ||
| import calendar | ||
| import unittest | ||
|
|
||
| import irods.test.helpers as helpers | ||
| import tempfile | ||
| from irods.session import iRODSSession | ||
| import irods.exception as ex | ||
| import irods.keywords as kw | ||
| from irods.ticket import Ticket | ||
| from irods.ticket import enumerate_tickets, Ticket | ||
| from irods.models import TicketQuery, DataObject, Collection | ||
|
|
||
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| # As with most of the modules in this test suite, session objects created via | ||
| # make_session() are implicitly agents of a rodsadmin unless otherwise indicated. | ||
| # Counterexamples within this module shall be obvious as they are instantiated by | ||
|
|
@@ -29,6 +34,17 @@ | |
| ) | ||
|
|
||
|
|
||
| def delete_tickets(session, dry_run = False): | ||
| for res in session.query(TicketQuery.Ticket): | ||
| t = Ticket(session, result=res) | ||
| if dry_run in (False, None): | ||
| t.delete(**{kw.ADMIN_KW: ""}) | ||
| elif isinstance(dry_run, list): | ||
| dry_run.append(t) | ||
| else: | ||
| logger.info('Found ticket: %s',t.string) | ||
|
|
||
|
|
||
| def delete_my_tickets(session): | ||
| my_userid = session.users.get(session.username).id | ||
| my_tickets = session.query(TicketQuery.Ticket).filter( | ||
|
|
@@ -358,6 +374,31 @@ | |
| os.unlink(file_.name) | ||
| alice.cleanup() | ||
|
|
||
| def test_new_attributes_in_tickets__issue_801(self): | ||
| # Specifically we are testing that 'modify_time' and 'create_time' attributes function as expected, | ||
| # and that other attributes such as 'id' are also present. | ||
| admin_ticket_for_bob = None | ||
|
|
||
| if (admin:=helpers.make_session()).server_version < (4, 3, 0): | ||
| self.skipTest('"create_time" and "modify_time" not supported for Ticket') | ||
|
Comment on lines
+382
to
+383
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This check isn't necessary since we no longer support anything earlier than 4.3.0.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok. we sure of that? I've heard concern from some on our team, in the not-so-distant past, that PRC may no longer be compatible with iRODS 3.0.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes. The 4.2 series is EOL which automatically makes iRODS 3 EOL. For users running 4.2, they have the ability to install earlier versions of the PRC and/or compile Python 3 if PRC 3 is needed. Of course, we don't make any guarantees about whether PRC 3 works with 4.2 deployments because that's not part of our testing. |
||
|
|
||
| try: | ||
| with self.login(self.bob) as bob: | ||
| bobs_ticket = Ticket(bob) | ||
| bobs_ticket.issue('write', helpers.home_collection(bob)) | ||
| time.sleep(2) | ||
| bobs_ticket.modify('add', 'user', admin.username) | ||
| bobs_ticket = Ticket(bob, result=enumerate_tickets(bob, raw=True), ticket=bobs_ticket.string) | ||
| self.assertGreaterEqual( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this be
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's correct. We're comparing If
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, I see... so... we're comparing self.assertGreater(bobs_ticket.modify_time, bobs_ticket.create_time) |
||
| bobs_ticket.modify_time, | ||
| bobs_ticket.create_time + datetime.timedelta(seconds=1) | ||
korydraughn marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| ) | ||
|
|
||
| admin_ticket_for_bob = Ticket(admin, result=enumerate_tickets(admin, raw=True), ticket=bobs_ticket.string) | ||
| self.assertEqual(admin_ticket_for_bob.id, bobs_ticket.id) | ||
| finally: | ||
| if admin_ticket_for_bob: | ||
| admin_ticket_for_bob.delete(**{kw.ADMIN_KW:''}) | ||
|
|
||
| class TestTicketOps(unittest.TestCase): | ||
|
|
||
|
|
@@ -455,7 +496,25 @@ | |
| def test_coll_ticket_write(self): | ||
| self._ticket_write_helper(obj_type="coll") | ||
|
|
||
|
|
||
| def test_enumerate_tickets__issue_120(self): | ||
|
|
||
| ses = self.sess | ||
|
|
||
| # t first assigned as a "utility" Ticket object | ||
| t = Ticket(ses).issue('read', helpers.home_collection(ses)) | ||
|
|
||
| # This time, t receives attributes from an internal GenQuery result. | ||
| t = Ticket( | ||
| ses, | ||
| result=enumerate_tickets(ses, raw=True), | ||
| ticket=t.string | ||
| ) | ||
|
|
||
| # Check an id attribute is present and listed in the results from list_tickets | ||
| self.assertIn(t.id, (_.id for _ in enumerate_tickets(ses))) | ||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| # let the tests find the parent irods lib | ||
| sys.path.insert(0, os.path.abspath("../..")) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,10 @@ | ||
| from irods.api_number import api_number | ||
| from irods.message import iRODSMessage, TicketAdminRequest | ||
| from irods.models import TicketQuery | ||
| from irods.column import Like, Column | ||
|
|
||
| from collections.abc import Mapping, Sequence | ||
| from typing import Any, Iterable, Optional, Type, Union | ||
|
Check failure on line 7 in irods/ticket.py
|
||
|
|
||
| import random | ||
| import string | ||
|
|
@@ -27,20 +31,86 @@ | |
| except ValueError: | ||
| raise # final try at conversion, so a failure is an error | ||
|
|
||
| class default_ticket_query_factory: | ||
|
Check failure on line 34 in irods/ticket.py
|
||
| _callable = staticmethod(lambda session: session.query(TicketQuery.Ticket)) | ||
| def __call__(self, session): | ||
| return self._callable(session) | ||
|
|
||
| def enumerate_tickets(session, *, query_factory=default_ticket_query_factory, raw=False): | ||
| """ | ||
| Enumerates (via GenQuery1) all tickets visible by, or owned by, the current user. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. GenQuery1 is an implementation detail. It's unlikely that anyone needs to know how the information is gathered.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Noted. Will change if desired. However, not mentioning it could confuse beginners, unless some work is done on the README. (Note for the "future me"? Should I explain what specific queries are incompatible with general queries? Or that Genquery2 will not suffice for this purpose until/unless the entire library is basically recast to be based on GenQuery2.)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Please say more.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, finally I decided I didn't need the mention. But, and I am asking because I'm curious if we are all tuned in to the inevitability that a removal of GenQuery1 would break the PRC at its foundation? It would need a re-design from the bottom of the foundation up. This raises the point: Do we plan on deprecating removing GenQuery1? Maybe around iRODS 7 or so? (Based on what I believe I've heard.)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The plan is to eventually remove GenQuery1 in the server, which would break all client code. Of course that might not be a reality for a very long time, e.g. iRODS 10, because we'd need full confidence that GenQuery2 covers everything. I don't think a grand redesign of the PRC will be required when that time comes because people write iRODS code in terms of function calls. It would mostly only require changing the internals of each call. There's nothing stopping us from offering a cleaner/smaller library for the python space today. Again, we'd have to continue supporting the existing implementation for a while since users have built important software on top of the PRC. Of course, we could offer automated tools for converting between the two libraries, but that requires a lot of work. |
||
|
|
||
| Args: | ||
| session: An iRODSSession object for use in the query. | ||
| query_factory: A class capable of generating a generic query or other iterable | ||
| over TicketQuery.Ticket row results. | ||
| raw: If false, transform each row returned into a Ticket object; else return | ||
| the result rows unaltered. | ||
|
|
||
| Returns: | ||
| An iterator over a range of ticket objects. | ||
| """ | ||
|
Check failure on line 52 in irods/ticket.py
|
||
| query = query_factory()(session) | ||
|
|
||
| if raw: | ||
| yield from query | ||
| else: | ||
| yield from (Ticket(session, result=row) for row in query) | ||
|
|
||
| _column_lookup = dict[Type[Column], Any] | ||
|
|
||
| class Ticket: | ||
| def __init__(self, session, ticket="", result=None, allow_punctuation=False): | ||
|
|
||
| def __init__(self, | ||
| session, | ||
| ticket="", | ||
| result: Optional[Union[_column_lookup, Iterable[_column_lookup]]] =None, # Optional (vs. '|') is Python 3.9 syntax | ||
| allow_punctuation=False): | ||
|
Comment on lines
+64
to
+68
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm finding this function hard to reason about, primarily in regard to
Maybe this constructor has too many responsibilities and splitting the logic across a few helper functions is all that's needed? For example: def __init__(self, session, ticket="", result:... = None, allow_punctuation=False):
if case_0_is_satisfied:
self._init_with_ticket(session, ticket)
elif case_1_is_satisfied:
self._init_with_ticket_and_result(session, ticket, result)
elif case_2_is_satisfied:
self._init_with_result(session, result)
raise Exception("Invalid args ...") Splitting up the code into smaller chunks let's you assume certain things are true moving forward. |
||
|
|
||
| self._session = session | ||
|
|
||
| try: | ||
| if result is not None: | ||
| if isinstance(result, Mapping): | ||
| if (single_string:=result.get(TicketQuery.Ticket.string, '')): | ||
| if ticket and (ticket != single_string): | ||
| raise RuntimeError( | ||
| f"The specified result contained a ticket string mismatched to the provided identifier ({ticket = })" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider rephrasing this statement. For example: f"No entry found in result argument matching ticket string ({ticket=})" |
||
| ) | ||
|
|
||
| elif hasattr(result, '__iter__'): | ||
| if ticket: | ||
| result = [row for row in result if row[TicketQuery.Ticket.string] == ticket][:1] | ||
|
Comment on lines
+81
to
+82
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can this fail if the ticket string doesn't match a row? |
||
|
|
||
| if not result: | ||
| result = None | ||
| else: | ||
| result = next(iter(result)) # result[0] | ||
|
|
||
| if result: | ||
| ticket = result[TicketQuery.Ticket.string] | ||
| for attr, value in TicketQuery.Ticket.__dict__.items(): | ||
| if value is TicketQuery.Ticket.string: continue | ||
| if not attr.startswith("_"): | ||
| try: | ||
| setattr(self, attr, result[value]) | ||
| except KeyError: | ||
| # backward compatibility with older schema versions | ||
| pass | ||
|
|
||
| self._ticket = ticket | ||
|
|
||
| except TypeError: | ||
| raise RuntimeError( | ||
| "If specified, 'result' parameter must be a TicketQuery.Ticket search result" | ||
| "If specified, 'result' parameter must be a TicketQuery.Ticket search result or iterable of same" | ||
korydraughn marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| ) | ||
| self._ticket = ( | ||
| ticket if ticket else self._generate(allow_punctuation=allow_punctuation) | ||
| ) | ||
|
|
||
| except IndexError: | ||
| raise RuntimeError( | ||
| "If both result and string are specified, at least one 'result' must match the ticket string" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should the user be able to specify both? Does this lead to deterministic behavior?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In a minor release I'd rather not change it, but I know it's confusing. It is admittedly confusing. Ticket objects exist in raw and cooked forms. The raw is for setup/creation, the cooked is to represent row results from a query. We can examine.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wait... if
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right. If result is the default of
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes. if it's a list of DB entries, ticket is used to filter. If there's only a single result passed in, that's used to produce a cooked object.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't you get a "cooked" object after filtering a list containing multiple items/rows too?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes, both are true. If the ticket string is given and there is a matching row, we get an object with the attributes filled in, ie the cooked object.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What happens if no row in I tried reading the implementation, but it wasn't clear to me if an error is raised or not. It appears you get an object containing the ticket string and that's it?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well , my intent is if you specify a ticket string and it isn't matched among the results, an error is raised. I'll make sure it's the case. I think, too, that is the logical expectation.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I agree. That makes the most sense to me. |
||
| ) | ||
|
|
||
| if not self._ticket: | ||
| self._ticket = self._generate(allow_punctuation=allow_punctuation) | ||
|
|
||
| @property | ||
| def session(self): | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's explicitly mention the attributes being tested here (i.e.
create_timeandmodify_time). These will not be "new" for very long.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fwiw,
idis also being tested!There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, whatever attributes are relevant. Or, if you prefer, an absolute moment in time: "attributes_unavailable_before_irods_4_3" or something like that.