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

Force finalize campaign #169

Merged
merged 8 commits into from
Sep 13, 2020
Merged

Conversation

baturin
Copy link
Contributor

@baturin baturin commented Jun 18, 2020

That pull request is aimed to start discussion on a problem when admin sees a lot of open campaigns which are not actual anymore.

Simple proposed solution in that PR is to allow closing campaigns regardless of their current state and active rounds. Could you please clarify what could be wrong with that approach?

@codecov-commenter
Copy link

codecov-commenter commented Jun 18, 2020

Codecov Report

Merging #169 into master will decrease coverage by 0.25%.
The diff coverage is 62.50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #169      +/-   ##
==========================================
- Coverage   76.52%   76.27%   -0.26%     
==========================================
  Files          19       19              
  Lines        3766     3793      +27     
  Branches      471      475       +4     
==========================================
+ Hits         2882     2893      +11     
- Misses        643      658      +15     
- Partials      241      242       +1     
Impacted Files Coverage Δ
montage/juror_endpoints.py 79.71% <50.00%> (-1.08%) ⬇️
montage/rdb.py 77.57% <60.86%> (-0.33%) ⬇️
montage/admin_endpoints.py 82.11% <69.23%> (-0.66%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a921c5d...e972d54. Read the comment docs.

@atsirlin
Copy link

atsirlin commented Jun 18, 2020

Could you please clarify what could be wrong with that approach?

Nothing is wrong in my opinion. The existing version does not allow to close the campaign unless it reached the ranking round, and this round has been finalized. However, many of the campaigns never reach the ranking round, either because organizers chose their own way and did the final ranking without Montage, or because the campaign was one of several campaigns created for a given contest (so the ranking round for this contest took place in a different campaign, if at all). Therefore, it is absolutely crucial to be able to close campaigns at any point of time. I have about 50 open campaigns on my list that I dream to close but I can't.

@mahmoud
Copy link
Member

mahmoud commented Jun 19, 2020

Sure. First off, thanks for this, I appreciate the initiative. This PR is a good starting point for discussion. Here are some thoughts:

It definitely makes sense to close a campaign if it's not proceeding any further. I'm just not sure if that should be called finalization. Finalization entails that there are final results stored somewhere in the system, and you can see that build_campaign_report, for instance, relies on the final round being a ranking round. This approach breaks that invariant.

Could a separate status make sense? Going from 3 to 4 statuses will mean revisiting invariants and transitions to make sure we handle the new status.

Personally, I'd definitely love this; Stephen and I see probably more campaigns than anyone else in the system, and if you thought the UI was slow before, phew! :)

@atsirlin
Copy link

@mahmoud: from the user's perspective it is just the 'closed campaign', no matter if it was really finalized or simply served its purpose. It is actually a wrong assumption that each grading should end with the ranking round. At least one third of the countries don't do that and choose their best photos based on the grades, or do the ranking without Montage. I would suggest to keep it simple for now: each campaign can be active or closed, and each admin can close or re-open the campaign.

@MichaelNMaggs
Copy link

MichaelNMaggs commented Jun 19, 2020

See also related issues #122 #144 and #157

The technical way in which previous campaign rounds have been finalised/finished/closed/dropped isn't normally of much interest to jurors. What concerns them is seeing huge numbers of old campaigns and rounds (tens in my case) that can't be hidden, take ages to load, and make it difficult to find the single active round that they're looking for. That's especially so since the active round always appears right at the end, and requires scrolling down to the bottom through all these 'dead' campaigns. If old things of no further interest could be hidden by the juror, that would solve it.

@mahmoud
Copy link
Member

mahmoud commented Jun 19, 2020

@atsirlin @MichaelNMaggs My reply was almost entirely technical in nature, relating the piggyback-on-finalize approach @baturin has proposed to hiding the campaigns. To be clear, I definitely understand the use case and want the feature, I just don't want more error cropping up down the line :)

@slaporte
Copy link
Member

I agree, I like this feature! The need to hide old campaigns is only getting greater every year that Montage is in use.

Could a separate status make sense? Going from 3 to 4 statuses will mean revisiting invariants and transitions to make sure we handle the new status.

Yes, I'd like to use the four statuses we have for rounds: active, finalized, paused (not active or finalized), and cancelled (basically, deleted). We don't do much with campaign status right now on the backend, so I can look into this.

@MichaelNMaggs raises an important point about improving load times. We should probably separate out the info for non-active campaigns from the /admin and /juror endpoint. It's an improvement to collapse non-active stuff, but it would be even better if users don't have to wait for it either.

@baturin
Copy link
Contributor Author

baturin commented Jun 20, 2020

Thanks for the discussion and your ideas! I have a couple of questions to clarify.

  1. Are we going to put campaigns which are not actually active (as they served their purpose) and which do no conform to the rule "last round is ranking and it is finalized" to "paused" status? Or we should set them to "finalized" status (as they are actually finished)?
  2. How can I cancel (set status to "cancelled") a campaign in UI? I don't see any controls responsible for that, may be I'm looking at the wrong place, could you describe what I should do? And then - why can't we just cancel all these campaigns?

@mahmoud
Copy link
Member

mahmoud commented Jun 20, 2020

Great questions. So I checked and sure enough, the UI must be lacking because production only has campaigns in active and finalized status. There is logic to cancel, but it must not have been used, and that may be for the better, because that logic also cancels rounds in that campaign, and thus votes underneath that. Other parts of the system ignore canceled votes, so I don't think we want that.

It doesn't have to work that way, of course, that's just how it would work without any other changes. One approach commonly taken in these positions is "active in the last 90 days". That might solve our problem, but if the converse, "paused and old", means "hide by default", how do users see those again? Maybe an archived flag?

@slaporte
Copy link
Member

Hmm. Is archived something we'd want to flag or just a display setting? We could conceivably check the last modified_date for any votes in a campaign, and then default to having then have them collapsed or less prominent after they are inactive for 90 days.

I am wondering if we even need a paused state for a campaign. I think we should decide if finalized means it must have results, or merely that it's done. One basic approach would be active, finalized, and cancelled, and then use flags to note which campaigns are finalized-with-report.

@@ -35,7 +35,7 @@ <h2 layout="row" layout-align="center center" ng-if="!$ctrl.campaign.id">
</md-button>
<md-button
Copy link
Member

Choose a reason for hiding this comment

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

I think we should also have a control option for re-activating a campaign.

@@ -45,11 +45,13 @@ function controller(
.getAdmin()
.then(data => {
vm.isAdmin = data.data.length;
const isActiveCampaign = campaign =>
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be based on campaign.status === 'active'

raise InvalidAction('only ranking rounds can be finalized')

campaign_summary = coord_dao.finalize_ranking_round(last_rnd.id)
campaign_summary = None
Copy link
Member

Choose a reason for hiding this comment

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

Should there be a different status for a campaign that's done but doesn't have a summary?

@@ -34,6 +34,8 @@ function controller($filter, $q, $state, $stateParams, adminService, alertServic
if (!vm.campaign.rounds.length) {
vm.campaign.rounds.push({});
}

vm.isCampaignClosed = vm.campaign.status === 'finalized';
Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link
Member

@slaporte slaporte left a comment

Choose a reason for hiding this comment

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

Thanks for submitting this, @baturin! I'm curious what you think about my suggestions below.

@baturin
Copy link
Contributor Author

baturin commented Jun 23, 2020

@slaporte @mahmoud thanks for your feedback.

Personally I think we should have 3 campaign statuses: 'active' and 'finalized', and optionally 'cancelled', where:

  • 'active' means our users are going to work with that campaign (create rounds, vote, view vote statistics, and so on)
  • 'finalized' means that campaign has served its purpose for users, regardless of results in database and regardless of the last round type
  • 'cancelled' means that campaign is garbage, e.g. created by mistake

That solution has the following advantages:

  1. It directly conforms to the actual business logic, described by @atsirlin
  2. It would give us a simple fast way to split campaigns we need from those which we don't need by simple SQL query, with no joins to rounds and other tables and no complex conditions. That would help us to resolve load time problem.

As a disadvantage, we need to find and fix all places like "build_campaign_report" mentioned by @mahmoud . That is mostly a quality risk, bugs may arise.

As an additional measure, I suggest to sort all campaigns by creation date (or last modification date?) in UI - that is already in PR.

@baturin
Copy link
Contributor Author

baturin commented Jun 23, 2020

After brief review I have not managed to find any way to view final report (which may depend on the last round results) from GUI, I see only API endpoints which are not used, and entrypoints like /admin/campaign/<campaign_id:int>/report, which are not referenced (still could be used by direct link). May be I'm checking at wrong places, could you please assist me?

@baturin
Copy link
Contributor Author

baturin commented Jul 23, 2020

@mahmoud @slaporte do you have any comments or updates on that PR? Thanks in advance!

@mahmoud
Copy link
Member

mahmoud commented Sep 8, 2020

Hey @baturin, wanted to revive this one over the holiday weekend. Noticed a few things

  1. By creating two new dashboard sections for archived campaigns, the component appears to be loading the same endpoints (and the same data) twice (based on the Network tab). In some cases this might be OK, but here, Toolforge is resource-strapped, so it'd be great if we could fetch just once, and filter out the archived data on the server unless explicitly requested by client. A "View Archive" approach as done with the Juror view could work better, too.
  2. There don't appear to be any backend tests, could we add a few of those to the pytest suite?

@atsirlin
Copy link

atsirlin commented Sep 8, 2020

@mahmoud: just a short comment. This pull request has been around for several months, but we can't discuss it forever, especially at such a slow pace. "Two dashboard sections" are not new, I have them on my screen since more than a year, and I did not see any major issue with the way they are organized or with their loading time. Let's focus on the main problem - that we need a solution to force close the campaigns. It may be a dirty solution for now, but it's better than having no solution for years.

@baturin
Copy link
Contributor Author

baturin commented Sep 10, 2020

Hi @mahmoud!

  1. As for two requests - the main dashboard already uses the same approach, sending two requests (one for admin, one for juror). I've simply reused that approach to keep code consistent, and keep API simple to use and test by splitting separate data over different requests.
  2. Sure, we could add some tests once we agree how the code should work :)

@mahmoud
Copy link
Member

mahmoud commented Sep 11, 2020

@baturin Got it. One request for admin and one for juror is understandable. I was seeing two for admin and two for juror.

Here's my thought, I appreciate @atsirlin's urgency, so I started a branch last night which shows Campaigns for the last 90 days and then instead of archive shows "All" (I think current/all vs. current/archived is a better pattern). It'll clean up the screen and get a faster load time. Coordinators who want to signal a campaign is cancelled or superseded can modify the name and it'll clear out of the screen over time. Because we've got some concerns over state and flags, I figured this would be the most expedient route.

Thoughts?

@atsirlin
Copy link

@mahmoud: I don't understand how it is supposed to work. Could you give an example?

@mahmoud
Copy link
Member

mahmoud commented Sep 12, 2020

@atsirlin so the way my local branch works is that campaigns with close dates 90 days in the past are not shown by default. You will have to click on "All Campaigns" to get to the full list. Not perfect; I'd still like to add the Archive flag and then change the default view to be "active in last 90 days + not archived" (and All continues to show all).

@mahmoud
Copy link
Member

mahmoud commented Sep 12, 2020

Stephen and I had a chat about this and we're going to make a run of building a proper "archive" (extending+testing the work in this PR). Will keep you posted.

@mahmoud
Copy link
Member

mahmoud commented Sep 13, 2020

Merging, continuing in #178 (Thanks @baturin and @atsirlin, see you there!)

@mahmoud mahmoud merged commit 883970d into hatnote:master Sep 13, 2020
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.

6 participants