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

Join and kick from all broadcast at the same time #13531

Merged
merged 16 commits into from
Sep 18, 2023

Conversation

kraktus
Copy link
Member

@kraktus kraktus commented Sep 3, 2023

close #12857 tested

Another option that probably would have reduced the code change would have been to check every time a join or kick was done If the study was belonging to a broadcast, but it seems nastier solution.

⚠️ Companion PR in Lila-ws: lichess-org/lila-ws#477

@ornicar
Copy link
Collaborator

ornicar commented Sep 17, 2023

I think I would prefer it the other way :-/ with the relay module listening to a study kick event.

Like here:

"study" -> { case lila.hub.actorApi.study.RemoveStudy(studyId) =>
api.onStudyRemove(studyId)
},

It would probably be a lot simpler and would not require a new lila-ws message

@kraktus
Copy link
Member Author

kraktus commented Sep 17, 2023

Ok 👍 Will do this week when I've time

@kraktus
Copy link
Member Author

kraktus commented Sep 17, 2023

done, now join and kick study events are monitored by relay, and if a matching broadcast (relaytour) is found, added to all rounds

Comment on lines 87 to 90
.foreach(_.foreach: tourId =>
api
.roundIdsById(tourId)
.foreach(_.foreach(studyId => studyApi.adminInvite(studyId, by))))
Copy link
Member Author

Choose a reason for hiding this comment

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

using foreach, I can't use .parallel anymore but I assume the futures will be parallelised

Copy link
Member Author

Choose a reason for hiding this comment

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

same thing as for kick, adminInvite is called twice for the original studyId when part of a broadcast, which eventually boils down to

  def admin(by: UserId)(study: Study): Funit =
    studyRepo.coll:
      _.update
        .one(
          $id(study.id),
          $set(s"members.${by}" -> $doc("role" -> "w", "admin" -> true)) ++
            $addToSet("uids" -> by)
        )
        .void

which is to be idempotent (sequence is a set). Maybe we want to add additional check though.

Comment on lines 116 to 118
applyWho: w =>
api.kick(studyId, username.id, w.u)
Bus.publish(actorApi.Kick(studyId, username.id, w.u), "kickStudy")
Copy link
Member Author

Choose a reason for hiding this comment

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

This mean api.kick is called twice for studyId when it's in a broadcast, but should be fine since api.kick check if the user is present.

* master:
  rename actors
  update pnpm-lock to fix CI
  remove debug
  comment study chapter auto-orientation choices
  simplify test assertions
  move looksLikePrize to modules/gathering
  select one post per blog then only shuffle
  remove unneeded pipe
  New Crowdin updates (lichess-org#13599)
  skip to branching points via keyboard nav
  add critical decision hopping to keyboard navigation
  maybe they wont notice yellow ones
  Update board menu to have consistent board flip
  Show activities for unlimited correspondence
  Fix prizeban for tournaments created with the tournament manager
  more tests for `String.looksLikePrize`
  shuffle before group
  minor frontpage blog tweak
it is eventually discarded in this specific case,
I still like it better that way
@ornicar ornicar merged commit 59db0ab into lichess-org:master Sep 18, 2023
3 checks passed
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.

The "enter as admin" button should enter all rounds of a broadcast
2 participants