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

(Contributor Recognition Infrastructure) Milestone 1.4: Add required email services to notify contributors about levels they achieved #15940

Merged
merged 30 commits into from
Sep 2, 2022

Conversation

chris7716
Copy link
Contributor

@chris7716 chris7716 commented Aug 24, 2022

Overview

  1. This PR fixes or fixes part of n/a.
  2. This PR does the following:

Essential Checklist

  • The PR title starts with "Fix #bugnum: ", followed by a short, clear summary of the changes. (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
  • The linter/Karma presubmit checks have passed locally on your machine.
  • "Allow edits from maintainers" is checked. (See here for instructions on how to enable it.)
    • This lets reviewers restart your CircleCI tests for you.
  • The PR is made from a branch that's not called "develop".

Proof that changes are correct

Proof of changes on desktop with slow/throttled network

Proof of changes on mobile phone

Proof of changes in Arabic language

PR Pointers

  • Make sure to follow the instructions for making a code change.
  • If you need a review or an answer to a question, and don't have permissions to assign people, leave a comment like the following: "{{Question/comment}} @{{reviewer_username}} PTAL". Oppiabot will help assign that person for you.
  • For what code owners will expect, see the Code Owner's wiki page.
  • Make sure your PR follows conventions in the style guide, otherwise this will lead to review delays.
  • Never force push. If you do, your PR will be closed.
  • Some of the e2e tests are flaky, and can fail for reasons unrelated to your PR. We are working on fixing this, but in the meantime, if you need to restart the tests, please check the "If your build fails" wiki page.

@chris7716 chris7716 requested review from a team as code owners August 24, 2022 18:04
@chris7716 chris7716 requested a review from a team August 24, 2022 18:04
@oppiabot
Copy link

oppiabot bot commented Aug 24, 2022

Hi @chris7716, can you complete the following:

  1. The karma and linter checklist has not been checked, please make sure to run the frontend tests and lint tests before pushing.
    Thanks!

@oppiabot
Copy link

oppiabot bot commented Aug 24, 2022

Hi @seanlip, @aks681 -- could one of you please add the appropriate changelog label to this pull request? Thanks!

@chris7716 chris7716 assigned seanlip and unassigned chris7716 Aug 29, 2022
Copy link
Contributor

@sagangwee sagangwee left a comment

Choose a reason for hiding this comment

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

@chris7716 LGTM

@oppiabot
Copy link

oppiabot bot commented Aug 29, 2022

Unassigning @sagangwee since they have already approved the PR.

@seanlip
Copy link
Member

seanlip commented Aug 30, 2022

@chris7716 Re #15940 (comment), please take a look at the email thread "GSoC project — product design feedback assessment" -- specifically the part saying "Our new rankings and levels". Is your current implementation consistent with that and are you planning for that structure in the rest of your PRs?

If so, I think it's probably fine, but I want to make sure that that is the case so that there are no surprises later.

Also, to be clear, I know it's not technically complicated to extend the object. But I think there are problems with figuring out e.g. what comes after "Legend" -- past a certain point, you run out of words. So you'll want the scale not to advance too quickly and that's where my concern comes from.

@seanlip seanlip assigned chris7716 and unassigned seanlip Aug 30, 2022
@chris7716
Copy link
Contributor Author

@seanlip yes I updated the constants according to the email thread and since ranks are mostly associated with the frontend, I will include those constants in the frontend related PRs. I will implement the logic for sending emails for achieving new ranks with the implementations of badges and ranks in the frontend. The logic to generate badges and sending emails for achieving new ranks will be pretty much the same.

I think we can go ahead with this PR and need to figure out terms for ranks before implementing frontend PRs.

@chris7716 chris7716 assigned seanlip and unassigned chris7716 Aug 30, 2022
Copy link
Member

@seanlip seanlip left a comment

Choose a reason for hiding this comment

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

All right, that sounds fine. Based on that understanding, I'm happy to give LGTM. Thanks!

@seanlip seanlip removed their assignment Aug 30, 2022
@chris7716
Copy link
Contributor Author

@aks681 Could you PTAL? Thanks!

@oppiabot oppiabot bot added the PR: don't merge - STALE BUILD The build on this PR is stale and should be restarted. label Sep 1, 2022
@oppiabot
Copy link

oppiabot bot commented Sep 1, 2022

Hi @chris7716, the build of this PR is stale and this could result in tests failing in develop. Please update this pull request with the latest changes from develop. Thanks!

@aks681 aks681 assigned chris7716 and unassigned aks681 Sep 1, 2022
@oppiabot oppiabot bot added the PR: LGTM label Sep 1, 2022
@oppiabot
Copy link

oppiabot bot commented Sep 1, 2022

Hi @chris7716, this PR is ready to be merged. Please address any remaining comments prior to merging, and feel free to merge this PR once the CI checks pass and you're happy with it. Thanks!

@oppiabot oppiabot bot removed the PR: don't merge - STALE BUILD The build on this PR is stale and should be restarted. label Sep 1, 2022
@chris7716
Copy link
Contributor Author

@seanlip I am not quite sure why these coverage tests are failling https://github.com/oppia/oppia/runs/8132243105?check_suite_focus=true

I updated the branch with the develop but still comes. I have not changes anything in those files though.

@seanlip
Copy link
Member

seanlip commented Sep 1, 2022

Would you please check in with @IamEzio and @U8NWXD about this? They are the members of the dev workflow team responsible for the backend tests.

@IamEzio
Copy link
Contributor

IamEzio commented Sep 2, 2022

Hi @chris7716! They were failing because some lines (1985-86) of core/domain/email_manager.py were not fully covered.

Screenshot from 2022-09-02 10-10-29

I think you covered them and the coverage checks are passing now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants