Implement vote and reveal & other improvements #8
Conversation
TiboX2021
changed the title
TiboX2021
added
enhancement
TiboX2021
force-pushed
the
implement-vote-and-reveal
branch
from
115fbcd
to
da4be6a
Compare
webapp/src/app/participant-view/participant-stage-view/exp-chat/exp-chat.component.ts
Outdated
Show resolved
Hide resolved
| this.messages.set(extendUntilMatch(this.messages(), m.reverse(), 'uid')); | ||
|
|
||
| // Find if new discuss items message have arrived | ||
| const last = m.find((m) => m.messageType === MessageType.DiscussItemsMessage) as |
There was a problem hiding this comment.
Don't use the name name for a list of things as a thing in the list :) (m in this case). Use a bit more detailed variable names and it'll be easier to understand.
There was a problem hiding this comment.
I'm also worried that this will simply find the last pair of items, not tell you if there's a new pair of items to discuss...
There was a problem hiding this comment.
renamed in 8ef5c53
There was a problem hiding this comment.
I track the current pair being discussed through the participants_ready_to_end_chat sync document, which also stores the index of the current pair being discussed in the preconfigured chat list. I should remove one of these 2 ways of computing the last pair being discussed, as they are redundant... If we want to keep the one that scans the last DIscussItemsMessage as the single source of truth, we could simply use a WritableSignal with lodash's deepEq comparison to make sure that the signal triggers on actual changes
| // } | ||
| // return isReady; | ||
| // }); | ||
| this.everyoneReachedThisStage = signal<boolean>(false); |
There was a problem hiding this comment.
Maybe add a comment to say that this is signal is expected to be thrown away and replaced OnInit when the stage data is set... ?
There was a problem hiding this comment.
Might be better to simply not have this initialized... otherwise you risk creating a dependency on the signal and then having the signal value replaced, but the dependency update forgotten about... Alternatively, whenever the stage is set, you write updates from the stage's signal to this signal... (that might be better, then you have the value always defined)
| this.everyoneReachedThisStage = this.participant.everyoneReachedCurrentStage(this.stage.name); | ||
| } | ||
|
|
||
| private queryClient = injectQueryClient(); |
There was a problem hiding this comment.
Needs a comment to say what this is I think.
There was a problem hiding this comment.
... I suspect this is not the right design... I think you should be simply be doing a firebase groupQuery for the participants, and then when they are all at done at this stage, you move to the next...
| // return sorted[0].userId; | ||
| // }); | ||
| nextStep() { | ||
| this.mutationReveal.mutate({ |
There was a problem hiding this comment.
I think we need a comment to say what/why we are doing this...
There was a problem hiding this comment.
yeah, the idea was to structure the stage flow like so:
- You fill the stage data as a participant
- You use a mutation to send your data to the backend / firestore
- If the mutation was successful, you navigate to the next page
However, I realized later on that participants are not supposed to be able to edit previous stages, so we should be able to simplify this (if the stage is being worked on by the participant, mutation -> navigation. Else, just navigate).
Because the LeaderReveal stage has no data to be filled by the participant, this mutation feels especially heavy (all that work just to update workingOnStageName), I will think of lighter alternatives
There was a problem hiding this comment.
oh I think I see, the point here is that you save the data to firestore at the time the person does the nextStep thing. Previously I did writes when data was edited... might be worth discussing how hard this is, and what the right abstraction is. My default would be that each stage kind has a class, and that class has functions to edit the data, and when edited, that results in a write to Firebase. So the key thing in a nextStep would be to call the corresponding stage instance's nextStep method that would simply mark this stage as complete.
TiboX2021
force-pushed
the
implement-vote-and-reveal
branch
from
678238a
to
27d9310
Compare
TiboX2021
force-pushed
the
implement-vote-and-reveal
branch
from
27d9310
to
ac1d3d3
Compare
|
@cjqian This PR is ready to be merged :) |
|
Wow, thank you! |
Finished the implementation of stages of type
voteForLeaderandleaderRevealin order to finish the transition from local demo to backend-based webapp.Updated the docker config for the webapp (documentation included in README.md)
Removed the hardcoded firebase configurations for deployment @ EPFL and included example configuration files (documented in README). Resolves Hide API key in a local file #7
Take into account remarks made in Merge back from the LSIR/llm--mediator-political repository #4's thread
Commit c838474 resolves #9
Firestore & cloud functions refactoring will be done in another PR