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.

Sign up for GitHub

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

Jump to bottom

Epic2 bugfix #825

Merged
merged 13 commits into from
Mar 20, 2017
Merged

Epic2 bugfix #825

merged 13 commits into from
Mar 20, 2017

Conversation

mattkrick
Copy link
Member

@mattkrick mattkrick commented Mar 15, 2017 •
edited
Loading

jordanh
jordanh reviewed Mar 15, 2017
View reviewed changes
src/universal/modules/meeting/helpers/actionMeeting.js
@@ -0,0 +1,58 @@
import {LOBBY, CHECKIN, UPDATES, FIRST_CALL, AGENDA_ITEMS, LAST_CALL, SUMMARY} from 'universal/utils/constants';

export default {
Copy link
Contributor

Choose a reason for hiding this comment

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

What you can't see, is the single tear rolling down my cheek.

Copy link
Member Author

Choose a reason for hiding this comment

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

SPAs grow up so fast these days...

@mattkrick
Copy link
Member Author

found a fun bug--
currently, we query for agendaItems when we start a meeting. that's how we know if all the agenda items have loaded yet.
However, if the team has 2 meetings back to back, then that old value is still cached. that means the meeting is broken the 2nd time around if the 2nd meeting has fewer agenda items than the first meeting.

we can't use a mutationHandler because we want to reset it for everyone.
so, we could do a few things:

  • completely refactor the meeting ephemeral data onto a meeting object, that way it'd have a distinct key in the client cache
  • trigger the query in the constructor so it happens everytime the meeting container gets mounted
  • trigger the requery ~10seconds after they go to the summary page (similar to a zero subscription, where we basically just tell it to refetch if certain data like SUMMARY comes through)
  • do nothing

@mattkrick
Copy link
Member Author

wooooo got it in with 8 mins to spare! ready for approval

@codecov
Copy link

codecov bot commented Mar 17, 2017 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (master@cf35e7f). Click here to learn what that means.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             master     #825   +/-   ##
=========================================
  Coverage          ?   58.48%           
=========================================
  Files             ?      120           
  Lines             ?     1650           
  Branches          ?        0           
=========================================
  Hits              ?      965           
  Misses            ?      685           
  Partials          ?        0
Impacted Files Coverage Δ
...graphql/models/Organization/addOrg/createNewOrg.js 100% <ø> (ø)
src/server/graphql/models/User/userSchema.js 38.46% <ø> (ø)
src/server/graphql/models/Team/teamSchema.js 33.33% <ø> (ø)
src/server/utils/authorization.js 38.54% <ø> (ø)
src/universal/utils/constants.js 100% <ø> (ø)
src/server/utils/serverConstants.js 100% <100%> (ø)

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 cf35e7f...f28065a. Read the comment docs.

jordanh
jordanh reviewed Mar 18, 2017
View reviewed changes
src/server/graphql/models/IntranetJobs/intranetJobsQuery.js
@@ -69,5 +71,26 @@ export default {
await Promise.all(adjustmentPromises);
return adjustmentPromises.length;
}
},
endOldMeetings: {
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah awesome, I will add this to action-chronos.

jordanh
jordanh requested changes Mar 18, 2017
View reviewed changes
Copy link
Contributor

@jordanh jordanh left a comment

Choose a reason for hiding this comment

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

Brilliant stuff @mattkrick, perhaps just a few comments are needed in spots?

src/universal/modules/meeting/helpers/electFacilitatorIfNone.js
import {cashay} from 'cashay';
import {showInfo} from 'universal/modules/toast/ducks/toastDuck';

const electFacilitatorIfNone = (nextProps, oldMembers) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this function could use some comments. I am having a hard time grokking what each of the state checks imply semantically. I'll give some examples:

src/universal/modules/meeting/helpers/electFacilitatorIfNone.js Outdated

const electFacilitatorIfNone = (nextProps, oldMembers) => {
const {dispatch, team: {activeFacilitator}, members} = nextProps;
if (!activeFacilitator) return;
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this mean that the meeting hasn't started yet and a facilitator has never been elected?

src/universal/modules/meeting/helpers/electFacilitatorIfNone.js Outdated
// if the meeting has started, find the facilitator
const facilitatingMemberIdx = members.findIndex((m) => m.isFacilitating);
const facilitatingMember = members[facilitatingMemberIdx];
if (!facilitatingMember || facilitatingMember.isConnected === true) return;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why isn't this facilitatingMember && facilitatingMember.isConnected === true?

src/universal/modules/meeting/helpers/electFacilitatorIfNone.js Outdated
if (!facilitatingMember || facilitatingMember.isConnected === true) return;
// check the old value because it's possible that we're trying before the message from the Presence sub comes in
const oldFacilitatingMember = oldMembers[facilitatingMemberIdx];
if (!oldFacilitatingMember || oldFacilitatingMember.isConnected === false) return;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is isConnected false here?

src/universal/modules/meeting/helpers/electFacilitatorIfNone.js Outdated
const onlineMembers = members.filter((m) => m.isConnected);
const callingMember = onlineMembers[0];
const nextFacilitator = members.find((m) => m.isFacilitator && m.isConnected) || callingMember;
if (callingMember.isSelf) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is super smart and needs no comment. Super clean way to make sure only one connected member calls the mutation :)

@jordanh
Copy link
Contributor

jordanh commented Mar 18, 2017 •
edited
Loading

Test Plan

  • Can reprioritize tasks on team dashboard when filter active
  • Can execute cron job for ending old meetings
  • Meeting facilitation:
    • Can start meeting
    • Second player can join
    • F can go forward through all phases, TMs follow
    • F can can go backward all phases, TMs follow
    • TMs can still diverge
    • Facilitator can drop (on browser close), new faciltator elected
    • Meeting summary is sent
  • highlighted teams work on sidebar
  • actions makes their appearance again
  • editing cursor annoyingly going to the end fixed/worked around

@jordanh
Copy link
Contributor

jordanh commented Mar 18, 2017 •
edited
Loading

So far @mattkrick, I could only find one reproducable issue #828.

I swear I also saw the agenda list not always mark items as completed, but I could be mistaken...

@mattkrick
Copy link
Member Author

that could be, i had to change the completion logic so maybe a bug got in, lemme play around more & see if i get some repro steps

@jordanh
Copy link
Contributor

jordanh commented Mar 20, 2017

Kk, I'm around and mostly available today so if you want to jam on it together lemme kno. Be sweet to get this up on prod in time for our meeting. Then we can all test together :)

@mattkrick
Copy link
Member Author

@jordanh can you give er another go & lemme know whatcha think?

@jordanh
Copy link
Contributor

jordanh commented Mar 20, 2017

LGTM!

@jordanh jordanh merged commit 7737568 into master Mar 20, 2017
@jordanh jordanh deleted the epic2-bugfix branch March 20, 2017 23:53
@jordanh jordanh removed the pr review label Mar 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jordanh jordanh jordanh requested changes

Assignees

@jordanh jordanh

Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@mattkrick @jordanh