Batch updates can't be applied to partner_genome under certain conditions

[anandaroop]

Original Slack report: https://artsy.slack.com/archives/C52PH2UCA/p1498156643769406

Just wanted to flesh this issue out here, so we can discuss and decide whether/how to proceed.

@reganartsy noticed that some batch gene removals didn't apply. In the example given:

• the gene being removed is Knitted and Crocheted
• there are four works for which the removal fails, all belonging to LEVEL Gallery
• the upstream error message from Gravity is Number of commercial categories exceeds limit (thanks @jonallured)

Turns out that the update fails because the partner has churned and therefore is currently allowed 0 categories, as opposed to 3, 4, or 5 for partners on Standard, Preferred or Premium plans:

gravity:staging> pp PartnerSubscriptionPlan.order(commercial_category_limit: -1).pluck(:name, :commercial_category_limit)
=> [
 ["Premium", 5],
 ["Plus", 4],
 ["Preferred", 4],
 ["Standard", 3],
 ["Folio", 0],
 ["Folio Unlimited", 0]
]

At this point any attempt to update the partner genome for this partner's works through the API will fail due to this validation.

So it's an edge case  — updating the partner genome of work belonging to a churned partner — but I'm not sure how edgy. (Note that this could even happen to a partner who merely downgrades their subscription plan, without churning)

@jonallured I wonder if we should try an attack this at the API level, like maybe skipping this validation if the update is coming from a genomer, along the lines of what we did in the API endpoint itself.

cc @reganartsy @madeleineb @jonallured

[jonallured]

Yeah, I could see that, something like this maybe?

  def update_partner_genome!(genes, current_user = nil)
    validate_genome(genes)
    user_is_genomer = current_user && current_user.roles.include?(User::GENOMER)
    validate_number_of_categories(genes) unless user_is_genomer
    partner_genome = updated_genome(genes)
    partner_genome.updated_by = current_user if current_user
    partner_genome.save!
    save!
  end

[anandaroop]

👍 Yeah I think that would do it.

It enforces the desired constraints for partners, but allows genomer updates to succeed if any of the following is true…

• the partner has churned
• the partner has downgraded subscription plans
• the resulting genome would somehow exceed the normal limit for the given partner's subscription plan (like maybe adding a gene during a multi-step cleanup operation?)

[jonallured]

I looked again and the PartnerGenomeUpdater only removes genes:

  def update
    return unless current_genes
    updated_genes = current_genes.except(*genes_to_remove)
    update_artwork_genes(updated_genes) if updated_genes != current_genes
  end

So, if the API were to be updated as we're suggesting above, then today Rosalind is a good API citizen and would only use it for good. 😝

But technically all we need for the API to do is accept gene removals regardless of how many categories the partner is allowed. The change above meets that need, but goes a step further by allowing additions as well. Maybe that's ok?

I was just trying to think of a way to reduce the surface here a little bit. Like, could we see if the before/after counts are not bigger and use that type of logic instead?

Or I could be overthinking it! 💭 💭 💭

[anandaroop]

Oh good point! Rosalind does not ever add to the partner genome, so it's reasonable to ask why we'd permit that.

I found the original suggestion above to be very straightforward and readable, with no real risk. But if you think we can match that, while capturing the suggested logic around removing genes, I'd be 👍 with that too.

[anandaroop]

I think we have an idea of how to approach this when the time comes.

@madeleineb — should we make a card to track/prioritize this on the big board?

[madeleineb]

@anandaroop let's circle up about this and then I can create a card/prioritize as need be! definitely depends if it affects published works for current partners, or not.