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

feat(client): improve SuperBlock cert claiming UX #41147

Merged

Conversation

ShaunSHamilton
Copy link
Member

Checklist:

  • I have read freeCodeCamp's contribution guidelines.
  • My pull request has a descriptive title (not a vague title like Update index.md)
  • My pull request targets the main branch of freeCodeCamp.
  • All the files I changed are in the same world language, for example: only English changes, or only Chinese changes, etc.

Closes #38569

Try to ignore the UI, and experience the UX. Same goes for the code. This is an POC

@ahmadabdolsaheb Please let me know if this is at all what you had in mind? The code is ugly, but if you like the UX, then I will continue with this, and refactor the whole component.

@ShaunSHamilton ShaunSHamilton added scope: UI Threads for addressing UX changes and improvements to user interface platform: learn UI side of the client application that needs familiarity with React, Gatsby etc. status: PR in works Work in Progress (WIP) Issues. labels Feb 17, 2021
@gitpod-io
Copy link

gitpod-io bot commented Feb 17, 2021

@ahmaxed
Copy link
Member

ahmaxed commented Feb 17, 2021

Thanks for coming up with the implementation quickly. Since we are deviating from the current flow of calming certifications, we would need a couple of approvals/feedback to make sure everyone is on the same page.

Let's finalize the discussion and implement the solution accordingly.

@ahmaxed
Copy link
Member

ahmaxed commented Feb 24, 2021

I think we reached a decision on #38569's solution. Let me know if you would need help with the implementation.

@ShaunSHamilton
Copy link
Member Author

Hey @ahmadabdolsaheb , I am going to need some help finding a non-convoluted way to calculate when someone is able to claim a cert (has submitted all 5 projects). Also, currently this will not work as expected for non-English challenges (anchor elements do not work).

@ahmaxed
Copy link
Member

ahmaxed commented Mar 4, 2021

@ShaunSHamilton thanks for your patience. We could use similar logic to what we are using in the settings page to check wether a cert is claimable or not (required projects have been completed or not) or simplify what we have there.

Also for the path to navigate to, it seems like we would have multiple scenarios that would require navigating to different places.

  1. user finished the last challenge on a superblock and has completed all required projects.

  2. user finished the last challenge on a superblock but has not completed all required projects.

  3. user has finished the last challenge on a superblock but has claimed the related certificate.

@ShaunSHamilton
Copy link
Member Author

Repost from Chat:
The issue is, how to tell someone can claim, without them trying to.

  • In /settings, a Camper will click the Claim button, which will send what looks like a PUT request to /certificate/verify, and respond with a success saying they have verified and claimed their cert.
  • For this, we just need to verify that the Camper has completed the necessary projects. Without claiming.

Essentially, I am asking if you can think of a better way to do this other than:
A) Relying on client-side calculation of how many projects are ticked
B) Creating a new endpoint in the api-server to verify without claiming. This would be doubly redundant, as with or without this, a Camper can still not necessarily claim a certificate without going through the current verification logic

@raisedadead Do you have any thoughts on if B should/should'nt be implemented?

@raisedadead
Copy link
Member

I am open to rejigging the API endpoints if needed.

@ShaunSHamilton
Copy link
Member Author

@ahmadabdolsaheb For these cases, where should the Camper be directed?

  1. user finished the last challenge on a superblock but has not completed all required projects.
  2. user has finished the last challenge on a superblock but has claimed the related certificate.

For both, should they be teleported to the next superblock (current impl)? Or, should they be taken to:

  • For 2 - to the project card to show not all projects have been completed
  • For 3 - to the claimed certification widget

@ShaunSHamilton
Copy link
Member Author

Current, before claiming:
image

After claiming:
image

@ahmaxed
Copy link
Member

ahmaxed commented Mar 8, 2021

For 2 - to the project card to show not all projects have been completed
For 3 - to the claimed certification widget

that makes sense

@ahmaxed
Copy link
Member

ahmaxed commented Mar 8, 2021

Current, before claiming:
image

After claiming:
image

It is coming along really well. thanks for the working on this feature.

The claim certification could take the whole row and be part of the card.

We could display the cert widget under the card.

@ShaunSHamilton
Copy link
Member Author

@ahmadabdolsaheb and @raisedadead , I am encountering some issues, due to the naming system we have (the maps and types):

  • My GET relies on finding the certType based on the superBlock:
let certType = superBlockCertTypeMap[superBlock];
  • The superBlock for the Scientific Computing with Python is scientific-computing-with-python
  • The superBlockCertTypeMap has it as scientific-computing-with-python-v7

How can this be changed, without breaking other things?

I notice the name property is undefined. Can this be filled with something useful?

isSciCompPyCertV7: {
    id: '5e44431b903586ffb414c951',
    tests: [ [Object], [Object], [Object], [Object], [Object] ],
    name: undefined,
    challengeType: 7
  },
  isDataAnalysisPyCertV7: {
    id: '5e46fc95ac417301a38fb934',
    tests: [ [Object], [Object], [Object], [Object], [Object] ],
    name: undefined,
    challengeType: 7
  },

Side note, Ahmad:

  • I am finding it difficult to navigate a Camper to the Claim Certification widget, once they complete the final project of a block.
  • The navigate is invoked, before the user data is updated that they have completed the 5 projects. So, they are navigated to the project section, instead.

@ahmaxed
Copy link
Member

ahmaxed commented Mar 11, 2021

  • The superBlock for the Scientific Computing with Python is scientific-computing-with-python
  • The superBlockCertTypeMap has it as scientific-computing-with-python-v7

That is a good questions, I think @scissorsneedfoodtoo put some effort to rename and normalize the challenge. Kris, do you know why we have a discrepancy among the superBlock names? and if normalizing the superBlock names in the curriculum/challenges/_meta would cause any issues.

I think you came across this issue before.

The navigate is invoked, before the user data is updated that they have completed the 5 projects. So, they are navigated to the project section, instead.

Generally speaking, we could wait for the action that updates the db to return successfully (or listen for its related action).

@scissorsneedfoodtoo
Copy link
Contributor

scissorsneedfoodtoo commented Mar 12, 2021

Wow, thank you for all your hard work on this @ShaunSHamilton. It looks like it's coming along great.

About the naming scheme in superBlockCertTypeMap, we added v7 to the certification flags for versioning, backward compatibility, and to prevent future conflicts. I believe we saw a potential conflict when we were splitting up the Quality Assurance certification and created the Legacy QA cert.

I looked through our old conversations about the changes to the flags / schema, and the consensus was that the versioning (v7, etc.) will be for internal use, and shouldn't be visible to campers in the UI or on the certifications. Instead, they'd just be in the DB and in the URL for the certification.

I'm not sure if we'd be able to normalize the super blocks in the curriculum/challenges/_meta/...json files because we might be using those to build the challenge map and the other pages. But it might be worth a shot.

What's interesting is that the createVerifyCert function has the same superBlockCertTypeMap[superBlock] code on this line, though the method of getting superBlock seems a bit different. Would modifying the createVerifyCanClaim function to do something similar be possible?

@ShaunSHamilton
Copy link
Member Author

What's interesting is that the createVerifyCert function has the same superBlockCertTypeMap[superBlock] code on this line, though the method of getting superBlock seems a bit different. Would modifying the createVerifyCanClaim function to do something similar be possible?

@scissorsneedfoodtoo Correct, but modifying it will do nothing, because it is exactly the same superBlock being passed around, AFAICS. The only difference is my use of a GET instead of POSTing it. A GET makes sense (in my mind), for this situation, but I do not see any change in the value of superBlock either way.

I will take a closer look later.

@ahmaxed
Copy link
Member

ahmaxed commented Mar 16, 2021

Changing those meta files should do it, but I suspect there are some components that read and display the superblock from the meta.

@ShaunSHamilton
Copy link
Member Author

ShaunSHamilton commented Mar 19, 2021

@ahmadabdolsaheb I now remember the issue with this; I was looking in the wrong place, before.

The issue is with the completion-epic.js meta information. It looks like this:

block: "scientific-computing-with-python-projects",
challengeType: 10,
helpCategory: "Python",
id: "5e444147903586ffb414c94f",
nextChallengePath: "/learn/scientific-computing-with-python/scientific-computing-with-python-projects/probability-calculator",
prevChallengePath: "/learn/scientific-computing-with-python/scientific-computing-with-python-projects/budget-app",
required: [],
superBlock: "scientific-computing-with-python",
template: null,
title: "Polygon Area Calculator",

Ideally, the meta would contain some information about the dev side - superBlock with -v7.

Can I open an issue about us using the same name superBlock for different things? It is constantly tripping me up, despite my constant working with the values.

client/src/utils/path-parsers.ts Outdated Show resolved Hide resolved
@ShaunSHamilton
Copy link
Member Author

Many thanks, Oliver. This appears to work as expected.

As such, other than adding tests, this PR is finally ready to go.

@ahmaxed
Copy link
Member

ahmaxed commented Jul 9, 2021

Thanks @ShaunSHamilton @ojeytonwilliams, looking forward to review the final iteration when it is ready.

@github-actions github-actions bot added the scope: tools/scripts Scripts for supporting dev work, generating config and build artifacts, etc. label Jul 9, 2021
@ShaunSHamilton
Copy link
Member Author

@ojeytonwilliams I cannot figure out why the final test is not working:
image

For some reason, the button is not clicked in the previous it

@ShaunSHamilton ShaunSHamilton removed the status: PR in works Work in Progress (WIP) Issues. label Jul 9, 2021
@raisedadead raisedadead added this to Merge Party in Next Jul 14, 2021
Copy link
Contributor

@scissorsneedfoodtoo scissorsneedfoodtoo left a comment

Choose a reason for hiding this comment

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

Everything LGTM 👍 👍

Copy link
Member

@moT01 moT01 left a comment

Choose a reason for hiding this comment

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

LGTM 🎉

@scissorsneedfoodtoo scissorsneedfoodtoo merged commit 6ca6d99 into freeCodeCamp:main Jul 15, 2021
Next automation moved this from In Team Review to Done Jul 15, 2021
@ShaunSHamilton ShaunSHamilton deleted the feat/claiming-cert branch July 15, 2021 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
platform: api Server application that needs familiarity with Express, Loopback, MongoDB etc. platform: learn UI side of the client application that needs familiarity with React, Gatsby etc. scope: i18n language translation/internationalization. Often combined with language type label scope: tools/scripts Scripts for supporting dev work, generating config and build artifacts, etc. scope: UI Threads for addressing UX changes and improvements to user interface
Projects
No open projects
Next
  
Done
Development

Successfully merging this pull request may close these issues.

Claiming certification after certificate completion
7 participants