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

Migrated commit_represent to S3Represent #1182

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Changes from all commits
Commits
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
96 changes: 65 additions & 31 deletions modules/s3db/req.py
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@
"req_req_list_layout",
"req_customise_commit_fields",
"req_commit_list_layout",
"req_CommitRepresent"
)

from gluon import *
Expand Down Expand Up @@ -2349,14 +2350,16 @@ def model(self):
msg_record_deleted = T("Commitment Canceled"),
msg_list_empty = T("No Commitments"))

commit_represent = req_CommitRepresent()

# Reusable Field
commit_id = S3ReusableField("commit_id", "reference %s" % tablename,
label = T("Commitment"),
ondelete = "CASCADE",
represent = self.commit_represent,
represent = commit_represent,
requires = IS_EMPTY_OR(
IS_ONE_OF(db, "req_commit.id",
self.commit_represent,
commit_represent,
orderby="req_commit.date",
sort=True)),
sortby = "date",
Expand Down Expand Up @@ -2423,35 +2426,6 @@ def model(self):
return dict(req_commit_id = commit_id,
)

# -------------------------------------------------------------------------
@staticmethod
def commit_represent(id, row=None):
"""
Represent a Commit

@ToDo: Migrate to S3Represent
"""

if row:
table = current.db.req_commit
elif not id:
return current.messages["NONE"]
else:
db = current.db
table = db.req_commit
row = db(table.id == id).select(table.type,
table.date,
table.organisation_id,
table.site_id,
limitby=(0, 1)).first()
if row.type == 1:
# Items
return "%s - %s" % (table.site_id.represent(row.site_id),
table.date.represent(row.date))
else:
return "%s - %s" % (table.organisation_id.represent(row.organisation_id),
table.date.represent(row.date))

# -------------------------------------------------------------------------
@staticmethod
def commit_onvalidation(form):
Expand Down Expand Up @@ -2764,6 +2738,66 @@ def commit_ondelete(row):
# @ToDo: Provide a way for the committer to specify this
db(rtable.id == req_id).update(commit_status=REQ_STATUS_NONE)

# -----------------------------------------------------------------------------
class req_CommitRepresent(S3Represent):
"""Representation of Commit"""
Copy link
Member

Choose a reason for hiding this comment

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

Blanks between """ and the text pls.


def __init__(self):
"""
Constructor
"""

table = current.s3db.req_commit
super(req_CommitRepresent, self).__init__()
Copy link
Member

Choose a reason for hiding this comment

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

This constructor does basically nothing - so should be removed.


def lookup_rows(self, key, values, fields=[]):
"""
Custom lookup method
@param key: the key Field
@param values: the values
@param fields: the fields to retrieve
"""

if len(values) == 1:
query = (key == values[0])
else:
query = key.belongs(values)

rows = current.db(query).select(self.table.type,
Copy link
Member

Choose a reason for hiding this comment

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

Critically, this lookup does not include table.id - but that is mandatory. All rows must contain the record ID, otherwise S3Represent won't work.

Copy link
Member

Choose a reason for hiding this comment

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

(How else could we identify which representation belongs to which id?)

This not being here tells me that you did not test it!

self.table.date,
Copy link
Member

Choose a reason for hiding this comment

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

Repeated lookups of self.table - should be brought into scope.

self.table.organisation_id,
self.table.site_id,
limitby=(0, len(values)))

commit_dates = [row[str(self.table.date)] for row in rows]
Copy link
Member

Choose a reason for hiding this comment

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

Repeated lookup of str(self.table.date) inside the loop - should be done once outside the loop (it's constant)

if commit_dates:
self.table.date.represent.bulk(commit_dates)
Copy link
Member

Choose a reason for hiding this comment

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

I don't think date.represent has a bulk-method?


organisation_ids = [row[str(self.table.organisation_id)] for row in rows]
Copy link
Member

Choose a reason for hiding this comment

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

Same - repeated lookup of str(self.table.organisation_id) inside the loop. Should be done once outside the loop.

if organisation_ids:
self.table.organisation_id.represent.bulk(organisation_ids)

site_ids = [row[str(self.table.site_id)] for row in rows]
Copy link
Member

Choose a reason for hiding this comment

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

Here too - same repeated lookup of a constant value inside the loop.

if site_ids:
self.table.site_id.represent.bulk(site_ids)

return rows

# -------------------------------------------------------------------------
def represent_row(self, row):
"""
Represent a single Row

@param row: the Row
"""

if row.type == 1:
return "%s - %s" % (self.table.site_id.represent(row.site_id),
Copy link
Member

Choose a reason for hiding this comment

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

You should test this - site_id.represent may return a link (unless you suppress it explicitly), and then this string construction is actually not ok, or is it?

self.table.date.represent(row.date))
else:
return "%s - %s" % (self.table.organisation_id.represent(row.organisation_id),
Copy link
Member

Choose a reason for hiding this comment

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

Same here - organisation_id.represent may return a link (unless you set show_link=False), which may not work with this type of string construction.

Copy link
Member

Choose a reason for hiding this comment

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

Subject to testing, of course - I'm not certain it really does.

self.table.date.represent(row.date))

# =============================================================================
class S3CommitItemModel(S3Model):
"""
Expand Down