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

TTO-276 Revise queueing order in CRMS #193

Merged
merged 3 commits into from
Nov 7, 2024
Merged

Conversation

moseshll
Copy link
Collaborator

@moseshll moseshll commented Nov 5, 2024

TL;DR for reviewer: a quick scan suffices, read on if fascinated with minutiae.

Note to reviewer: this has been tested on dev-2. This is a somewhat inconsequential tweak to the runtime behavior of the system. Basically we want to promote "mdp" volumes to the front of the queue and the front of the candidates. PresentationOrder is an existing mechanism (note camel case) and queue_order is the new method (note snake case) using almost the same mechanism, just applied to a different query. (The mechanism is embarrassingly clunky but I don't know how else to do it: basically we allow a Project to respond with its own ORDER BY clause that gets plugged into the caller's query.)

I have an issue (#195) to change the return values of these subclass methods. Out of scope for this ticket.

  • Add a custom PresentationOrder to the Core project to prioritize mdp namespace.
  • Add a new Project#queue_order used the same way as PresentationOrder except:
    • It applies to the list of candidates being considered for addition to the Queue.
    • Like PresentationOrder the custom ORDER BY becomes the first order clause.
    • And the default behavior is to return undef.

- Add a custom `PresentationOrder` to the Core project to prioritize mdp namespace.
- Add a new `Project#queue_order` used the same way as `PresentationOrder` except:
  - It applies to the list of candidates being considered for addition to the Queue.
  - Like `PresentationOrder` the custom `ORDER BY` becomes the first order clause.
@moseshll moseshll marked this pull request as draft November 5, 2024 15:35
my $sql = 'SELECT * FROM projects WHERE id=?';
my $ref = $self->{'crms'}->get('dbh')->selectall_hashref($sql, 'id', undef, $id);
$self->{$_} = $ref->{$id}->{$_} for keys %{$ref->{$id}};
if (defined $id) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added this so we could create a Project instance without an id (primary key). Normally we never instantiate the Project class, just the subclasses, but this gives me a chance to test the default behavior without writing tests for all of the subclasses (definitely out of scope for this ticket).

cgi/CRMS.pm Show resolved Hide resolved
@moseshll moseshll requested a review from mwarin November 6, 2024 19:58
@moseshll moseshll marked this pull request as ready for review November 6, 2024 19:58
Copy link

@mwarin mwarin left a comment

Choose a reason for hiding this comment

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

Looks fine, nothing that stands in the way of an APPROVE.

cgi/CRMS.pm Show resolved Hide resolved
cgi/Project.pm Outdated Show resolved Hide resolved
cgi/CRMS.pm Show resolved Hide resolved
@moseshll moseshll merged commit de9a872 into main Nov 7, 2024
1 check passed
@moseshll moseshll deleted the TTO-276_core_mdp_to_the_front branch November 7, 2024 19:45
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