Skip to content

fix: first section celebration #977

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
Merged
leangseu-edx merged 1 commit into from
Feb 14, 2023
Merged

fix: first section celebration #977

leangseu-edx merged 1 commit into from
Feb 14, 2023

Conversation

@dyudyunov
Copy link
Contributor

@dyudyunov dyudyunov commented Sep 14, 2022

fix: first section celebration

Fix the first section celebration modal showing logic.

On Nutmeg+ it's shown only after the page reload or after going directly
to the second section from the course home. Going through the course
with the Next/Previous buttons has no effect (which worked on Maple).

Notes:

  • the weekly goal has the same showing logic, but I assume that is
    correct behavior so no changes are added for it in this commit.
  • showing a celebration modal for the first section completion when
    going directly to the first unit of the second section seems to be a bug
    (reproduces on Maple too)

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Sep 14, 2022
@openedx-webhooks
Copy link

openedx-webhooks commented Sep 14, 2022

Thanks for the pull request, @dyudyunov! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

@dyudyunov
Copy link
Contributor Author

dyudyunov commented Sep 14, 2022

this is a backport of #976

@natabene
Copy link

natabene commented Sep 19, 2022

@dyudyunov Thank you for the contribution, looks like this is ready.

@dyudyunov
Copy link
Contributor Author

dyudyunov commented Sep 19, 2022

@dyudyunov Thank you for the contribution, looks like this is ready.

yes 👍

@dyudyunov
Copy link
Contributor Author

dyudyunov commented Oct 4, 2022

Hi @natabene
Can you tell if this one was planned for review?

@natabene
Copy link

natabene commented Oct 4, 2022

@dyudyunov It is lined up but I don't know when team will have bandwidth for this.

@itsjeyd itsjeyd mentioned this pull request Oct 14, 2022
Open
4 tasks
@ProductRyan
Copy link

ProductRyan commented Dec 5, 2022

internal tracking link:
https://2u-internal.atlassian.net/browse/AU-946

muselesscreator
muselesscreator reviewed Dec 5, 2022
View reviewed changes
Copy link
Contributor

@muselesscreator muselesscreator left a comment

Choose a reason for hiding this comment

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

Please add tests for these new behaviors.

muselesscreator
muselesscreator suggested changes Dec 5, 2022
View reviewed changes
Copy link
Contributor

@muselesscreator muselesscreator left a comment

Choose a reason for hiding this comment

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

Please add tests for the new behaviors

@itsjeyd
Copy link

itsjeyd commented Dec 15, 2022

@dyudyunov Please let us know when this is ready for review again. I'll mark it as waiting-on-author in the meantime, like I just did for #976.

@itsjeyd itsjeyd added the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Dec 15, 2022
@dyudyunov dyudyunov force-pushed the nutmeg/fix-first-section-celebration branch from 4153175 to 7f119b7 Compare December 19, 2022 12:49
@openedx-webhooks openedx-webhooks removed the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Dec 19, 2022
@dyudyunov
Copy link
Contributor Author

dyudyunov commented Dec 19, 2022

hi @itsjeyd @muselesscreator

please, run checkers and review

@itsjeyd itsjeyd added the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Dec 21, 2022
@codecov
Copy link

codecov bot commented Dec 21, 2022 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (open-release/nutmeg.master@8e1904a). Click here to learn what that means.
Patch has no changes to coverable lines.

Additional details and impacted files 
@@                      Coverage Diff                      @@
##             open-release/nutmeg.master     #977   +/-   ##
=============================================================
  Coverage                              ?   84.08%           
=============================================================
  Files                                 ?      258           
  Lines                                 ?     4450           
  Branches                              ?     1154           
=============================================================
  Hits                                  ?     3742           
  Misses                                ?      690           
  Partials                              ?       18

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@itsjeyd itsjeyd removed the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Dec 22, 2022
@itsjeyd
Copy link

itsjeyd commented Dec 22, 2022

@muselesscreator A quick heads-up that this is now ready for review, too. (Checks ran successfully.)

@itsjeyd
Copy link

itsjeyd commented Jan 6, 2023

Hey @muselesscreator, a friendly nudge about reviewing this PR.

@jmakowski1123 jmakowski1123 added the product review PR requires product review before merging label Jan 10, 2023
@itsjeyd
Copy link

itsjeyd commented Jan 11, 2023

@muselesscreator A quick heads-up that this PR was tagged for product review so it might actually make sense to hold off on technical review for now.

CC @jmakowski1123

@dyudyunov
fix: first section celebration
87df5ab 
Fix the first section celebration modal showing logic.

On Nutmeg+ it's shown only after the page reload or after going directly
to the second section from the course home. Going through the course
with the Next/Previous buttons has no effect (which worked on Maple).

Notes:
- the weekly goal has the same showing logic, but I assume that is
correct behavior so no changes are added for it in this commit.
- showing a celebration modal for the first section completion when
going directly to the first unit of the second section seems to be a bug
(reproduces on Maple too)
@dyudyunov dyudyunov force-pushed the nutmeg/fix-first-section-celebration branch from 7f119b7 to 87df5ab Compare January 11, 2023 10:31
@itsjeyd itsjeyd added the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Jan 12, 2023
@itsjeyd itsjeyd mentioned this pull request Jan 17, 2023
Merged
@itsjeyd
Copy link

itsjeyd commented Jan 31, 2023

@ProductRyan Once product review for the main PR (#976) is done, could you please approve this PR (Nutmeg backport) and #1044 (Olive backport) as well?

@dyudyunov We'll move forward with engineering review after getting 👍 from product here.

@jmakowski1123 jmakowski1123 removed the product review PR requires product review before merging label Jan 31, 2023
@jmakowski1123 jmakowski1123 added the product review PR requires product review before merging label Jan 31, 2023
@itsjeyd
Copy link

itsjeyd commented Feb 8, 2023

@ProductRyan I'll consider this backport approved from the product perspective, but let me know if you have any objections.

@itsjeyd
Copy link

itsjeyd commented Feb 8, 2023

@dyudyunov Once #976 gets a green build, could you make sure that this PR is up-to-date with all the latest changes from #976?

Then we'll be able to give this to the engineering team again for a final look.

CC @muselesscreator

@itsjeyd itsjeyd added waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. and removed needs test run Author's first PR to this repository, awaiting test authorization from Axim labels Feb 8, 2023
leangseu-edx
leangseu-edx approved these changes Feb 14, 2023
View reviewed changes
Copy link
Contributor

@leangseu-edx leangseu-edx left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@leangseu-edx leangseu-edx merged commit 66482ab into openedx:open-release/nutmeg.master Feb 14, 2023
@openedx-webhooks
Copy link

openedx-webhooks commented Feb 14, 2023

@dyudyunov 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

@dyudyunov dyudyunov deleted the nutmeg/fix-first-section-celebration branch February 15, 2023 08:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@muselesscreator muselesscreator Awaiting requested review from muselesscreator

1 more reviewer

@leangseu-edx leangseu-edx leangseu-edx approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

open-source-contribution PR author is not from Axim or 2U product review PR requires product review before merging waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc.

Projects

Contributions
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@dyudyunov @openedx-webhooks @natabene @ProductRyan @itsjeyd @muselesscreator @leangseu-edx @jmakowski1123