Skip to content
Snippets Groups Projects

Create a way to inject promotions into listing results #59

Merged Create a way to inject promotions into listing results #59
github/fork/autumnfound/malowe/master/59 into master
Merged Martin Lowe requested to merge github/fork/autumnfound/malowe/master/59 into master

Updated application to add promotion service. Added promotion to DTO layer. Added call to listings fetch to inject promotions in certain conditions. Refactored code to make URL parameters more robust. Reduced duplication by setting ID matching in DtoFilters to be default functionality.

Signed-off-by: Martin Lowe martin.lowe@eclipse-foundation.org

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Christopher Guindon
  • Christopher Guindon
  • Christopher Guindon
    Christopher Guindon @cguindon started a thread on commit 10ba4ae7
    • src/main/java/org/eclipsefoundation/marketplace/service/impl/DefaultPromotionService.java
    123 LOGGER.debug("Could not find any promos to inject");
    124 } else {
    125 for (Listing listing : getListingsForPromotions(wrapper, promoHolding)) {
    126 LOGGER.debug("Injecting promo with listing ID '{}' ", listing.getId());
    127 listing.setPromotion(true);
    128 listings.add(0, listing);
    129 }
    130 }
    131 }
    132
    133 /**
    134 * Using the weighting set in the promotions (or default if not set), retrieve a
    135 * random result, taking weighting into account.
    136 *
    137 * @param promos list of promotions to retrieve a result from. This list will be
    138 * modified as part of this call to shuffle and pop the chosen
    • Christopher Guindon
      Christopher Guindon @cguindon ·
      Maintainer

      Created by: mbarbero

      This is a bad contract. Never expect clients to read the doc :). Program defensively and not modify your inputs. Do a copy as the first step in your method and remove this contract from the doc.

  • Christopher Guindon
  • Christopher Guindon
  • Christopher Guindon
  • Christopher Guindon
    Christopher Guindon @cguindon started a thread on commit 10ba4ae7
    • src/main/java/org/eclipsefoundation/marketplace/service/impl/DefaultPromotionService.java
    98 if (!cachedResults.isPresent() || cachedResults.get().isEmpty()) {
    99 LOGGER.debug("Could not find any promotions to inject, returning");
    100 return;
    101 }
    102 // make a copy of the array to not impact cached values
    103 List<Promotion> promos = new ArrayList<>(cachedResults.get());
    104 List<Promotion> promoHolding = new ArrayList<>(promoCount);
    105 LOGGER.debug("Found {} promotions, maximum number to inject {}", promos.size(), promoCount);
    106 // check each promotion to see if it should be injected
    107 Promotion curr;
    108 while ((curr = getWeightedPromotion(promos)) != null && promoHolding.size() <= promoCount) {
    109 // create a local final field referencing the current promotion for stream ref
    110 final Promotion p = curr;
    111 LOGGER.debug("Checking promo {}", p.getListingId());
    112 // check if current promo matches any of the listing IDs
    113 if (promos.stream().noneMatch(l -> l.getId().equals(p.getListingId()))) {
    • Christopher Guindon
      Christopher Guindon @cguindon ·
      Maintainer

      Created by: mbarbero

      So, curr is added to promoHolding only if there is no other promotion left in the promos list that have the same listingId? Is that correct? If yes, why?

  • Christopher Guindon
  • Martin Lowe
    Martin Lowe @malowe started a thread on commit 10ba4ae7
    • src/main/java/org/eclipsefoundation/marketplace/service/impl/DefaultPromotionService.java
    98 if (!cachedResults.isPresent() || cachedResults.get().isEmpty()) {
    99 LOGGER.debug("Could not find any promotions to inject, returning");
    100 return;
    101 }
    102 // make a copy of the array to not impact cached values
    103 List<Promotion> promos = new ArrayList<>(cachedResults.get());
    104 List<Promotion> promoHolding = new ArrayList<>(promoCount);
    105 LOGGER.debug("Found {} promotions, maximum number to inject {}", promos.size(), promoCount);
    106 // check each promotion to see if it should be injected
    107 Promotion curr;
    108 while ((curr = getWeightedPromotion(promos)) != null && promoHolding.size() <= promoCount) {
    109 // create a local final field referencing the current promotion for stream ref
    110 final Promotion p = curr;
    111 LOGGER.debug("Checking promo {}", p.getListingId());
    112 // check if current promo matches any of the listing IDs
    113 if (promos.stream().noneMatch(l -> l.getId().equals(p.getListingId()))) {
    • Martin Lowe
      Martin Lowe @malowe ·
      Author Maintainer

      The current check will compare the current promotions listing ID with the listings that we were passed. It will only add the promo to the holding list if the listing it represents isn't already in the list. This way we won't get promotions for something we'd already see. We can remove this logic, but it means we might see the same listing twice

  • Martin Lowe
  • Martin Lowe
    Martin Lowe @malowe started a thread on commit 10ba4ae7
    • src/main/java/org/eclipsefoundation/marketplace/service/impl/DefaultPromotionService.java
    123 LOGGER.debug("Could not find any promos to inject");
    124 } else {
    125 for (Listing listing : getListingsForPromotions(wrapper, promoHolding)) {
    126 LOGGER.debug("Injecting promo with listing ID '{}' ", listing.getId());
    127 listing.setPromotion(true);
    128 listings.add(0, listing);
    129 }
    130 }
    131 }
    132
    133 /**
    134 * Using the weighting set in the promotions (or default if not set), retrieve a
    135 * random result, taking weighting into account.
    136 *
    137 * @param promos list of promotions to retrieve a result from. This list will be
    138 * modified as part of this call to shuffle and pop the chosen
    • Martin Lowe
      Martin Lowe @malowe ·
      Author Maintainer

      I agree that unless you make it explicit (like putting terms in your signature) this can be a dangerous bit of code, even if its private. Would naming it popWeightedPromo() be a suitable compromise?

  • Martin Lowe
  • Christopher Guindon
    Christopher Guindon @cguindon ·
    Maintainer

    Created by mbarbero

    Review: Approved

  • Christopher Guindon
    Christopher Guindon @cguindon ·
    Maintainer

    Merged by: mbarbero at 2020-02-13 11:16:49 UTC

    • Please register or sign in to reply

    Copyright © Eclipse Foundation, Inc. All Rights Reserved.     Privacy Policy | Terms of Use | Copyright Agent