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

Fix program id retrieval when sharing #2061

Merged
merged 3 commits into from
Jan 23, 2024
Merged

Fix program id retrieval when sharing #2061

merged 3 commits into from
Jan 23, 2024

Conversation

annagav
Copy link
Contributor

@annagav annagav commented Jan 16, 2024

What are the relevant tickets?

Fix #2056

Description (What does it do?)

Fixes a bug when getting the program id.

How can this be tested?

Go to view your program record, click enable sharing, check that sharing request was called with the correct program id.

Screenshot 2024-01-16 at 3 37 15 PM

@@ -77,7 +77,7 @@ export class LearnerRecordsPage extends React.Component<Props, State> {
}

getOnlyProgramId() {
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the difference between getOnlyProgramId and getProgramId?

Copy link
Contributor Author

@annagav annagav Jan 19, 2024

Choose a reason for hiding this comment

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

I also had that question. I think the getOnlyProgramId does the same thing getProgramId without the check for return this.props.match.params.program.length !== 36. But we should ask @jkachel for the meaning behind it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

were you able to test "Allow Sharing" on the program records?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, this works for me. But that mystery "36" in the similar function should be figured out before approving.

Copy link
Contributor

Choose a reason for hiding this comment

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

The URLs are the same if you're viewing for a program or a shared record. The shared records use a UUID, hence 36. Neither of these two functions are named particularly well; getProgramId is really a check for a shared program record where getOnlyProgramId only returns if there's a program ID (aka an int).

Copy link
Contributor

@jkachel jkachel left a comment

Choose a reason for hiding this comment

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

Functionality works well but I think these two functions should be renamed to something more representative of what they do. These should only be used here so that should be a quick refactor.

Copy link
Contributor

@collinpreston collinpreston left a comment

Choose a reason for hiding this comment

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

So I have a program with 2 required courses. I also have a program certificate. When I load the program record page I am seeing an error in the console and the program sharing buttons are not displayed. When I switch back to the main branch, this does not occur.

? this.props.match.params.program
: false
isProgramRecordShared() {
return this.props.match.params.program.length === 36
Copy link
Contributor

Choose a reason for hiding this comment

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

I would add a comment indicating what the 36 number is and how it's used.

@annagav annagav requested a review from collinpreston January 19, 2024 21:29
Copy link
Contributor

@collinpreston collinpreston left a comment

Choose a reason for hiding this comment

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

Very good!

Copy link
Contributor

@jkachel jkachel left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@annagav annagav merged commit 11a33c3 into main Jan 23, 2024
3 checks passed
@annagav annagav deleted the ag/get_program_id branch January 23, 2024 16:34
@odlbot odlbot mentioned this pull request Jan 23, 2024
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Program record shows with missing courses for the old DEDP program
3 participants