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

Fixes part of #18442: Remove deprecated edit state content suggestion model #20079

Conversation

masterboy376
Copy link
Collaborator

@masterboy376 masterboy376 commented Mar 29, 2024

Overview

  1. This PR fixes part of [Feature Request]: Deprecate SuggestionEditStateContent #18442.
  2. This PR does the following: Introduces a new beam job to delete deprecated edit state content suggestion from datastore.

Essential Checklist

  • The PR title starts with "Fix #bugnum: " or "Fix part of #bugnum: ...", followed by a short, clear summary of the changes.
  • I have followed the instructions for making a code change.
  • I have assigned the correct reviewers to this PR (or will leave a comment with the phrase "@{{reviewer_username}} PTAL" if I don't have permissions to assign reviewers directly).
  • The linter/Karma presubmit checks pass on my local machine, and my PR follows the coding style guide).
  • "Allow edits from maintainers" is checked. (See here for instructions on how to enable it.)

Testing doc (for PRs with Beam jobs that modify production server data)

  • A testing doc has been written: Testing doc
  • (To be confirmed by the server admin) All jobs in this PR have been verified on a live server, and the PR is safe to merge.

Proof that changes are correct

Screenshot (130)

PR Pointers

Copy link

oppiabot bot commented Mar 29, 2024

Hi @vojtechjelinek, @DubeySandeep, @kevintab95, PTAL at this PR, it modifies files in jobs or platform folders.
Also @masterboy376, please make sure to fill in the server jobs form for the new job or feature to be tested on the backup server. This PR can be merged only after the test run is successful. Please refer to this guide for details.
Thanks!

@oppiabot oppiabot bot added the PR: Affects datastore layer Labels to indicate a PR that changes the datastore layer. label Mar 29, 2024
Copy link

oppiabot bot commented Mar 29, 2024

Assigning @kevintab95 for the first pass review of this PR. Thanks!

@masterboy376
Copy link
Collaborator Author

@seanlip
Copy link
Member

seanlip commented Mar 30, 2024

@masterboy376 I have not seen this testing doc before, but it is listed as me being the server admin and all the dropdowns in the reviewer checklist are marked as Yes. However I don't think this has actually been reviewed by any of us yet (I don't even have access to edit the doc).

Please read and follow the instructions in the release testing doc template carefully, and do not self-approve things and attribute those approvals to other people. That is not OK. (I am also not supposed to be the server admin for this.)

@masterboy376
Copy link
Collaborator Author

masterboy376 commented Mar 30, 2024

@masterboy376 I have not seen this testing doc before, but it is listed as me being the server admin and all the dropdowns in the reviewer checklist are marked as Yes. However I don't think this has actually been reviewed by any of us yet (I don't even have access to edit the doc).

Please read and follow the instructions in the release testing doc template carefully, and do not self-approve things and attribute those approvals to other people. That is not OK. (I am also not supposed to be the server admin for this.)

Pardon @seanlip, I misunderstood the testing doc instruction before. I have fixed it. I am working on 2 issues and a project proposal currently. I assure this won't repeat. PTAL

@masterboy376
Copy link
Collaborator Author

@kevintab95 and @U8NWXD , just polite reminder PTAL.

Copy link
Member

@DubeySandeep DubeySandeep left a comment

Choose a reason for hiding this comment

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

LGTM for changes in the (2) codeowner files!

Copy link

oppiabot bot commented Mar 30, 2024

Unassigning @DubeySandeep since they have already approved the PR.

@U8NWXD U8NWXD removed their assignment Mar 30, 2024
Copy link

oppiabot bot commented Apr 15, 2024

Unassigning @kevintab95 since the review is done.

Copy link

oppiabot bot commented Apr 15, 2024

Hi @masterboy376, it looks like some changes were requested on this pull request by @kevintab95. PTAL. Thanks!

@masterboy376
Copy link
Collaborator Author

Hello @kevintab95 addressed your review comments. PTAL

@oppiabot oppiabot bot assigned kevintab95 and unassigned masterboy376 Apr 15, 2024
Copy link

oppiabot bot commented Apr 15, 2024

Unassigning @masterboy376 since a re-review was requested. @masterboy376, please make sure you have addressed all review comments. Thanks!

Copy link
Member

@kevintab95 kevintab95 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @masterboy376!

Copy link

oppiabot bot commented Apr 15, 2024

Unassigning @kevintab95 since they have already approved the PR.

@oppiabot oppiabot bot added the PR: LGTM label Apr 15, 2024
Copy link

oppiabot bot commented Apr 15, 2024

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

Copy link
Contributor

@chris7716 chris7716 left a comment

Choose a reason for hiding this comment

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

Assigning my self as the server admin to run this job

@chris7716 chris7716 self-assigned this Apr 16, 2024
Copy link

oppiabot bot commented Apr 16, 2024

Unassigning @chris7716 since the review is done.

@oppiabot oppiabot bot removed the PR: LGTM label Apr 16, 2024
Copy link

oppiabot bot commented Apr 16, 2024

Hi, @chris7716, the LGTM Label has been removed because the changes were requested on this PR. Thanks!.

Copy link
Contributor

@chris7716 chris7716 left a comment

Choose a reason for hiding this comment

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

Ran it successfully on backup server.

@chris7716 chris7716 added this pull request to the merge queue Apr 17, 2024
@oppiabot oppiabot bot added the PR: LGTM label Apr 17, 2024
Copy link

oppiabot bot commented Apr 17, 2024

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

Merged via the queue into oppia:develop with commit d21272a Apr 17, 2024
77 checks passed
@masterboy376 masterboy376 deleted the remove_deprecated_edit_state_content_suggestion_model branch April 17, 2024 14:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: Affects datastore layer Labels to indicate a PR that changes the datastore layer. PR: LGTM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants