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

Add back reverted quizzes: run-a-node, scaling, solo staking #11960

Merged
merged 6 commits into from
Jan 29, 2024
Merged

Conversation

wackerow
Copy link
Member

@wackerow wackerow commented Jan 18, 2024

Description

Three quizzes were previously added, which surfaced some bugs requiring them to be reverted:

This PR re-adds the above quizzes now that the underlying bugs have been fixed.

Small note, renamed the run-a-node keys to use l instead of i to prevent a future conflict with #11884

Preview links

Copy link

netlify bot commented Jan 18, 2024

Deploy Preview for ethereumorg ready!

Name Link
🔨 Latest commit 0813e43
🔍 Latest deploy log https://app.netlify.com/sites/ethereumorg/deploys/65b18b9ddfa16400086e7028
😎 Deploy Preview https://deploy-preview-11960--ethereumorg.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@github-actions github-actions bot added content 🖋️ This involves copy additions or edits review needed 👀 translation 🌍 This is related to our Translation Program labels Jan 18, 2024
Copy link
Member

@pettinarip pettinarip left a comment

Choose a reason for hiding this comment

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

The code looks good. Marking this as blocked after testing.

While this does not crash as it did before (#10828), it does not update the stats correctly.

======
How to reproduce error:

  1. Go to /quizzes
  2. Open the devtools and manually set the following user stats in localstorage (tab Application):

This will simulate a user that has already used the quizzes.

key: quizzes-stats
value: {"score":3,"average":[60],"completed":"{\"what-is-ethereum\":[false,0],\"what-is-ether\":[false,0],\"web3\":[false,0],\"wallets\":[false,0],\"security\":[false,0],\"nfts\":[false,3],\"layer-2\":[false,0],\"merge\":[false,0]}"}

  1. Reload the page
  2. Make the "Run a node" quiz
  3. After you finish the quiz, notice that the stats are not updated

cc @TylerAPfledderer pinging you in case you have any idea of what might be happening

initialize "completed" to empty object, and add score fallback if not yet taken
@wackerow
Copy link
Member Author

wackerow commented Jan 19, 2024

@pettinarip I was able to get this working by setting a fallback value when setting lastScore inside useQuizWidget. Also changed the initial completed object to just an empty object {}. With this, I'm having success locally with this updating any existing data from local storage as new quizzes are taken.


Of note, I did have issues with the exact string you pasted for testing, as it contains escaped characters. Did you copy this directly from your ls? When I go to https://ethereum.org/quizzes with a cleared locale storage, I do not get this type of string on my end (it populates without the escapes):

{"score":5,"average":[100],"completed":{"what-is-ethereum":[true,5],"what-is-ether":[false,0],"web3":[false,0],"wallets":[false,0],"security":[false,0],"nfts":[false,0],"layer-2":[false,0],"merge":[false,0]}}

If I go to https://ethereum.org/quizzes, and paste in the locale storage directly from above, I get this:
image
Notice 169/8 completed... if I clear this and let it do its thing, this does not happen. Thus, I don't think we need to worry about it, just noting... maybe you can confirm on your end as well.

@wackerow
Copy link
Member Author

Separately, while debugging this I ended up improving the type-safety on all of these Quiz components while trying to figure out what was going on... in the end, it was not necessary for this fix, but since the work was done I went ahead and posted this in a separate PR, #11992

@wackerow wackerow requested a review from pettinarip January 19, 2024 21:52
@TylerAPfledderer
Copy link
Contributor

@wackerow @pettinarip I'm seeing a completely different issue with the local storage.

If I clear my local storage, and do a quiz in the quiz hub through this PR's preview deploy, I see this data structure:
image

This happened if I did the same thing on the Run A Node page.

@pettinarip
Copy link
Member

@TylerAPfledderer I tested it in FF and it worked fine for me. Does the quiz still work? could it just be a devtool issue? Seems that it detects that completed is an Object but it is displaying it as an array 🤔

@pettinarip
Copy link
Member

@wackerow nice! tested it again and it worked fine! gj.

Regarding the encoding issue, this is interesting. So, I might have some pretty old quiz data in my localstorage (must be from the first version of the quiz hub logic). I guess we encoded that part differently back then. Now we are not doing that anymore, so I guess its better now!

*/
const INITIAL_COMPLETED_QUIZZES: CompletedQuizzes = Object.keys(
allQuizzesData
).reduce((object, key) => ({ ...object, [key]: [false, 0] }), {})
Copy link
Member

Choose a reason for hiding this comment

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

Interesting. We don't need to initialize everything. Simpler, I like it.

src/components/Quiz/useLocalQuizData.ts Show resolved Hide resolved
const lastScore = prevStats.completed[quizKey][1]
const { completed } = prevStats
const hasResultsSaved = !!completed[quizKey]
const lastScore = hasResultsSaved ? prevStats.completed[quizKey][1] : 0
Copy link
Member

Choose a reason for hiding this comment

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

Nice.

@pettinarip
Copy link
Member

@wackerow @TylerAPfledderer

While testing it again, I was able to reproduce Tyler's bug/issue. So, the encoded data version generates the problem described by Tyler and as a result shows the wrong 169/8 completed stat in the UI mentioned by Paul.

I'm not sure how we could elegantly fix this by supporting both versions: the encoded (old) and the decoded one (new).

Perhaps we would need to add new check to detect if the previous data is encoded and decode it, and then continue with the normal flow.

Comment on lines 16 to 22
// If the user has an old version of the app, convert the
// `completed` value from a string to an object.
const [current, setCurrent] = data
if (typeof current.completed === "string") {
const newCompleted = JSON.parse(current.completed)
setCurrent({ ...current, completed: newCompleted })
}
Copy link
Member Author

Choose a reason for hiding this comment

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

@pettinarip @TylerAPfledderer This should fix the issues with the escaped strings for the completed value. This was happening because the completed object was being stringified inside a parent also being stringified.

This simply checks if .completed is of string type, and if it is, parses it and re-assigns as an object to the locale storage value.

// If "current" value looks like this:
{"score":3,"average":[60],"completed":"{\"what-is-ethereum\":[false,0],\"what-is-ether\":[false,0],\"web3\":[false,0],\"wallets\":[false,0],\"security\":[false,0],\"nfts\":[false,3],\"layer-2\":[false,0],\"merge\":[false,0]}"}

// ...this code block will convert it to this:
{"score":3,"average":[60],"completed":{"what-is-ethereum":[false,0],"what-is-ether":[false,0],"web3":[false,0],"wallets":[false,0],"security":[false,0],"nfts":[false,3],"layer-2":[false,0],"merge":[false,0]}}

Just need to double check no edge cases if we have this local storage data and then take a stand-alone quiz, but hopefully this fixes the backward compatibility.

Copy link
Member

Choose a reason for hiding this comment

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

Great! yeah, I guess we will be forced to do this for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like it's fixed on my end!

Copy link
Contributor

Choose a reason for hiding this comment

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

This might also help with the setup with the QuizManager waiting to come online (To have the quiz components isolated and to be supplied separate groups of quiz data) as well as that issue where adding quizzes or quiz questions to the data set compromised existing user stats and throwing id errors.

Does this help with the later?

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 fix should take care of compatibility issues when adding new quizzes to the data set.

Copy link
Member

@pettinarip pettinarip left a comment

Choose a reason for hiding this comment

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

Awesome! working fine now for me with old and new data structures 💪🏼

@TylerAPfledderer
Copy link
Contributor

@wackerow @pettinarip I would like to note here for future design consideration that with the standalone pages, maybe have a way to show the user they already completed the quiz with 100%? Maybe a separate visual that is similar to the summary component?

@wackerow
Copy link
Member Author

I would like to note here for future design consideration that with the standalone pages, maybe have a way to show the user they already completed the quiz with 100%? Maybe a separate visual that is similar to the summary component?

Yeah, I like that idea... cc: @nloureiro ^

@nloureiro
Copy link
Contributor

I would like to note here for future design consideration that with the standalone pages, maybe have a way to show the user they already completed the quiz with 100%? Maybe a separate visual that is similar to the summary component?

Yeah, I like that idea... cc: @nloureiro ^

Do you understand the idea is to save the user's last interaction on a specific page? Regardless if he refreshes the page.

  • If I get 100% it would show this
  • if I have 50% it would show this and push to redo again

is that it?

@TylerAPfledderer
Copy link
Contributor

I would like to note here for future design consideration that with the standalone pages, maybe have a way to show the user they already completed the quiz with 100%? Maybe a separate visual that is similar to the summary component?

Yeah, I like that idea... cc: @nloureiro ^

Do you understand the idea is to save the user's last interaction on a specific page? Regardless if he refreshes the page.

  • If I get 100% it would show this
  • if I have 50% it would show this and push to redo again

is that it?

That's correct, instead of the quiz always starting over on refresh

@pettinarip pettinarip merged commit 35ebc05 into dev Jan 29, 2024
9 of 10 checks passed
@pettinarip pettinarip deleted the quizzes branch January 29, 2024 12:09
This was referenced Jan 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
content 🖋️ This involves copy additions or edits translation 🌍 This is related to our Translation Program
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants