close #874 - implement selection of previous iterations for reviewer and user #923

Ulisseus · 2021-06-30T01:03:57Z

This PR is very big, but unfortunately I cannot break it down into smaller pieces, everything is tightly coupled together. I could try to separate changes for reviewer and student pages but it would lead to weird temporary components and helpers which would be overwritten by the very next pr. The initial pr would be about the same size minus a few files.

Main changes are in ReviewCard and ChallengeMaterial components. I'd suggest starting review with ReviewCard, it's shorter and a bit easier to read.

ReviewCard

Currently we're using GetSubmissions query to fetch all open submission for the specified lesson. This PR adds additional query GetPreviousSubmissions which is triggered for every ReviewCard component once it mounts. It's non-blocking data fetching, so render speed will be exactly the same as before.

Is it possible to fetch everything in one query? I don't know, I guess some sql wizard could have done it, but Prisma doesn't support subqueries yet, and the raw sql would be really big and clunky to maintain.

There are two new states - index and submission. Submission is the current selected submission and index is its index in getPreviousSubmissionQuery data.

The initial component got too big, so I split it in two. Review buttons are now in their own ReviewButtons component, their functionality is the same.

ChallengeMaterial

ChallengeQuestionCard component also got too big. Diff rendering is now done in a separate component - ChallengeQuestionCardDisplay.

Again, there are two new states - index and submission. But unlike ReviewCard there are two useEffects too. One is for updating data after receiving getPreviousSubmissions query and changing current index just like in ReviewCard. Another one is for resetting index when user clicks on different challenge.

updateCache

Both components use the same updateCache helper function which updates getPreviousQuery client side.

For people who are not familiar with updating client cache in apollo. Only server side is updated when you use a mutation, so you need to manually override cache in order to reflect current state of component. Once you do this all updated queries-hooks are triggered which results in rerendering. It's a data loop of sorts.

DiffView

This component had a lot of different props but they all were parts of submissions type. So istead of adding another one I simply passed the whole submission. GeneralStatus is total status of submission (for example, you could have five iterations from getPreviousSubmissions query - 'needMoreWork', 'overwritten', 'overwritten', 'needMoreWork', 'Open') with generalStatus 'Open'.

CommentBox

Again, the same generalStatus. Also, I choose to show comments even on rejected submissions, hiding them makes no sense when you can select previous submissions. Adding comments is disabled when generalStatus isn't 'Open' though.

Test file changes

I added new dummy data file - getPreviousSubmissionsData. We should unify our test data someday, currently most tests use their own data. It makes test files harder to read but more importantly if you update any grapql type you need to update every test mock (grapqh enforces types even in tests).

Vercel (I created test submission under js0, do not comment on students previous iterations yet, users won't be able to see your messages).

vercel · 2021-06-30T01:04:01Z

@Ulisseus is attempting to deploy a commit to a Personal Account owned by @garageScript on Vercel.

@garageScript first needs to authorize it.

codecov · 2021-06-30T01:06:53Z

Codecov Report

Merging #923 (4af51cc) into master (17e6818) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #923   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          112       112           
  Lines         1921      1957   +36     
  Branches       475       486   +11     
=========================================
+ Hits          1921      1957   +36

Impacted Files	Coverage Δ
components/SelectIteration.tsx	`100.00% <ø> (ø)`
graphql/queryResolvers/getPreviousSubmissions.ts	`100.00% <ø> (ø)`
helpers/controllers/userInfoController.ts	`100.00% <ø> (ø)`
components/ChallengeMaterial.tsx	`100.00% <100.00%> (ø)`
components/CommentBox.tsx	`100.00% <100.00%> (ø)`
components/DiffView.tsx	`100.00% <100.00%> (ø)`
components/ReviewCard.tsx	`100.00% <100.00%> (ø)`
helpers/updateCache.ts	`100.00% <100.00%> (ø)`

vercel · 2021-06-30T01:07:09Z

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/c0d3/c0d3-app/KLp3ZFB6rq4HTFUQDMu6UzvDz93Q
✅ Preview: https://c0d3-app-git-fork-ulisseus-iterations-review-c0d3.vercel.app

Ulisseus · 2021-06-30T01:11:09Z

components/ReviewCard.tsx

+  useEffect(() => {
+    if (data?.getPreviousSubmissions) {
+      setPreviousSubmissions(data)
+      /*istanbul ignore else*/


I spent quite a lot of time trying to trigger this else case but to no avail. I couldn't get the final rerender from updating the cache in tests no matter what.

I've managed to finally trigger that branch by hardcoding comment for every cache update. I guess it's better than completely ignoring it.

components/ChallengeMaterial.tsx

tomrule007 · 2021-06-30T03:59:17Z

components/ChallengeMaterial.tsx

  const comments =
-    currentChallenge.submission &&
-    currentChallenge.submission.comments?.filter(comment => !comment.line)
+    submission && submission.comments?.filter(comment => !comment.line)


Could you remove the submission && .... and just do submission?.comments?.filter(comment => !comment.line)

Since we are using lodash, I think it is better to use _.get lodash function to maintain codebase consistency. With lodash, I don't think this repo needs to use the ?. operator.

_.get(submission, 'comments', []).filter(...)

Tom is right, looking at this part I am not sure why I needed submissions&& check in the first place.

Chaining ? feels more natural than using lodash in this case. If anything we should start replacing _.get with ? operators everywhere.

Agreed on using the coalescing chain. It is more natural and works better with typescript. Lodash uses a string for the property which may cause bugs if the name changes for some reason.

Ya I would vote moving to the coalescing operator when possible. A quick global search on the project for _.get( returns 40 hits in 13 files, seems like we should be able to refactor it out of the codebase at some point in the future.
* This could be a very jr. opinion, as I haven't seen how bad projects can get when the consistency slowly gets destroyed with promises of future refactoring.

Agreed on sprint Honeydew to use and replace _.get with the coalescing chain operator ?..

components/ChallengeMaterial.tsx

components/ReviewCard.test.js

Ulisseus · 2021-06-30T07:16:15Z

There is something off with rejection comment on vercel preview, the last open submission should not have rejection message. I'll check it tomorrow, this is not the intended behavior.

ggwadera · 2021-06-30T13:12:17Z

As a small suggestion, please add a screenshot of the new or changed component when you're working on front-end code. It's easier to grasp what the PR is about this way. I know that there is the storybook preview but it's easier if you have a little screenshot on your description.

That said, I think the current styling has way too much whitespace, and that weird box around it.

Maybe the submissions selector could go on the top right, after the challenge name, and make it a little smaller and the boxes more close together. Also that Iteration 3 of 4 text is kinda redundant since it's the same information as the boxes display.

By the way, the review comment editor is still shown for past iterations. I think it could be hidden to avoid any confusion that could happen, like someone submitting a review thinking the submission is wrong but the newest one is correct.

Ulisseus · 2021-06-30T20:54:39Z

please add a screenshot of the new or changed component when you're working on front-end code

Maybe I should have added general layout with this component. But storybook enforces types, so it's rather cumbersome creating all these dummy files according to proper type definitions.

Anyways, selection counter is now in the top right corner. And accept/reject buttons are hidden for previous submissions (check updated vercel).

ggwadera · 2021-07-01T00:07:33Z

components/ChallengeMaterial.tsx

-    username!
-  )
+
+  const { data, loading, error } = useGetPreviousSubmissionsQuery({


Why not reduce this query to only get the past submissions ids and then fetch them on demand by its id? Querying them all here with the diff will increase the loading time when opening the review page (meaning the full screen loading spinner).

I am not sure how it should work. GetPreviousSubmisisons query doesn't affect fullscreen loading spinner at all.

Current fetching order is:

getApp query (it should be in client's cache, and if this is not the case user is being redirected)

getSubmission query fetches all open submissions for this lesson (this is what actually hidden behind fullscreen loading spinner)

Review page is rendered. getPreviousSubmissions query starts, select iteration in the top right corner is showing "Loading submissions...".

getPreviousSubmissions returns and submission selection is fully rendered. So it doesn't affect loading spinner duration at all, it only increases time to final render but most users probably won't mind it (nobody notices a small component in the top right corner).

Alternatives:

Fetch everything in one query. First, I don't know how to do it (it's not possible in prisma and raw sql would be really big). Second, and more important this would actually increase loading spinner duration while reducing time to final render. This tradeoff would lead to bad UX in my opinion, why wait for full data when you can render it bit by bit and making page look more responsive?

Use lazy loading. This again would be a worse UX. When you click on iteration you need to wait for query to return. Why not fetch it on first render while user is focused on reading initial diff?

Come to think about it, we need to get rid of fullscreen spinners someday. Nothing here prevents rendering of NavBar, Footer and Lesson components. Maybe even add a template loading submission just like youtube does it with gray rectangles.

Yeah you're right, my mistake. We'll have to see how it will work out with big files like on JS6, the submission selector could take a longer time to load since it will have to fetch the entire submissions.

Come to think about it, we need to get rid of fullscreen spinners someday. Nothing here prevents rendering of NavBar, Footer and Lesson components. Maybe even add a template loading submission just like youtube does it with gray rectangles.

Yes having a skeleton placeholder would be ideal.

ggwadera · 2021-07-03T12:47:31Z

Accepted submissions without comment are not showing who was the reviewer.

Should look like this one but without the approval comment text.

Ulisseus · 2021-07-03T16:20:29Z

Accepted submissions without comment are not showing who was the reviewer.

It should work correctly even without a comment. Did you check the old preview? Oh, I forgot to update vercel link in the first message.

I need to work more on these review cards though. Maybe add another one for resubmission. And refactor them into separate component to use in challenge page.

But this PR is too big already, so I'd rather make another one for these changes.

Ulisseus added 5 commits June 30, 2021 01:11

add submission selection to reviewer and student

cefe331

add generalStatus and conditional rendering of Add comment button

d440c2f

add tests

c56801c

remove type assertions for name/username

d928b6b

fix request changes message

23706df

vercel bot deployed to Preview June 30, 2021 01:07 View deployment

Ulisseus commented Jun 30, 2021

View reviewed changes

tomrule007 reviewed Jun 30, 2021

View reviewed changes

components/ChallengeMaterial.tsx Outdated Show resolved Hide resolved

tomrule007 reviewed Jun 30, 2021

View reviewed changes

components/ChallengeMaterial.tsx Outdated Show resolved Hide resolved

tomrule007 reviewed Jun 30, 2021

View reviewed changes

components/ReviewCard.test.js Outdated Show resolved Hide resolved

Ulisseus added 2 commits June 30, 2021 09:49

fix requestChanges rendering

f7fb67c

fix a typo in import

ddba5e9

vercel bot deployed to Preview June 30, 2021 07:02 View deployment

Ulisseus added 2 commits June 30, 2021 21:10

add acceptance message in reviews

efdf7f7

change selectIteration css, hide review buttons for old submissions

41414d2

vercel bot deployed to Preview June 30, 2021 20:24 View deployment

set sorting order for getPreviousSubmissions

566af4a

vercel bot deployed to Preview June 30, 2021 20:42 View deployment

ggwadera reviewed Jul 1, 2021

View reviewed changes

Ulisseus added 2 commits July 1, 2021 21:39

remove istanbul ignore

d94dff7

update styling of rejection message/iteration components

d62bfbc

vercel bot deployed to Preview July 1, 2021 20:12 View deployment

fix date in acceptance/rejection messages

4af51cc

vercel bot deployed to Preview July 1, 2021 20:20 View deployment

kondanna approved these changes Jul 4, 2021

View reviewed changes

ggwadera approved these changes Jul 4, 2021

View reviewed changes

ggwadera merged commit 5f5988d into garageScript:master Jul 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

close #874 - implement selection of previous iterations for reviewer and user #923

close #874 - implement selection of previous iterations for reviewer and user #923

Ulisseus commented Jun 30, 2021 •

edited

Loading

vercel bot commented Jun 30, 2021

codecov bot commented Jun 30, 2021 •

edited

Loading

vercel bot commented Jun 30, 2021 •

edited

Loading

Ulisseus Jun 30, 2021

Ulisseus Jul 1, 2021

tomrule007 Jun 30, 2021

anthonykhoa Jun 30, 2021 •

edited

Loading

Ulisseus Jun 30, 2021

ggwadera Jun 30, 2021

tomrule007 Jul 1, 2021

ggwadera Jul 4, 2021

Ulisseus commented Jun 30, 2021

ggwadera commented Jun 30, 2021 •

edited

Loading

Ulisseus commented Jun 30, 2021

ggwadera Jul 1, 2021

Ulisseus Jul 1, 2021

ggwadera Jul 3, 2021

ggwadera commented Jul 3, 2021

Ulisseus commented Jul 3, 2021 •

edited

Loading

close #874 - implement selection of previous iterations for reviewer and user #923

close #874 - implement selection of previous iterations for reviewer and user #923

Conversation

Ulisseus commented Jun 30, 2021 • edited Loading

ReviewCard

ChallengeMaterial

updateCache

DiffView

CommentBox

Test file changes

vercel bot commented Jun 30, 2021

codecov bot commented Jun 30, 2021 • edited Loading

Codecov Report

vercel bot commented Jun 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anthonykhoa Jun 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ulisseus commented Jun 30, 2021

ggwadera commented Jun 30, 2021 • edited Loading

Ulisseus commented Jun 30, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ggwadera commented Jul 3, 2021

Ulisseus commented Jul 3, 2021 • edited Loading

Ulisseus commented Jun 30, 2021 •

edited

Loading

codecov bot commented Jun 30, 2021 •

edited

Loading

vercel bot commented Jun 30, 2021 •

edited

Loading

anthonykhoa Jun 30, 2021 •

edited

Loading

ggwadera commented Jun 30, 2021 •

edited

Loading

Ulisseus commented Jul 3, 2021 •

edited

Loading