[Feature Request]: Deprecate SuggestionEditStateContent

[seanlip]

Is your feature request related to a problem? Please describe.

We used to have functionality where users could suggest changes/improvements to an exploration through the exploration player. The suggestions received via this functionality were not useful, so we deprecated the feature.

However, we still have SuggestionEditStateContent domain objects in the codebase and similar suggestions in the datastore. The aim of this issue is to track the removal of those items completely so that we don't need to maintain that code any more.

Describe the solution you'd like

Safely delete all "edit state content" suggestion models, as well as the SuggestionEditStateContent domain object and any code that relates to "edit state" suggestions (including in the exploration editor feedback tab).

[DubeySandeep]

@seanlip Is it a duplicate of #15975 ?

[seanlip]

@DubeySandeep Oops, you are right, thanks a lot! I closed the other one since this issue has a bit more detail. Thanks!

[masterboy376]

Hello @seanlip, To solve this issue first i will write a beam job to delete all the "edit state content suggestion" model from datastore. Then will delete SuggestionEditStateContent domain object and other related code. I have started working on the beam job and will soon showcase it.

[seanlip]

Sounds good, thanks @masterboy376. One note though: make sure to do some analysis to ensure that the models you're deleting aren't referred to by other models (so that we don't run into errors when those other models get fetched). You can leave a comment here to explain what steps you took to do the analysis (in a way that others can reproduce).

Thanks!

[masterboy376]

Hello @seanlip,
Upon analyzing, the list of backend files that are using GenerralSuggestionModel with suggestion_type
equals to feconf.SUGGESTION_TYPE_EDIT_STATE_CONTENT:

• core/storage/suggestion/gae_models.py
• core/storage/suggestion/gae_models_test.py
• core/domain/wipeout_service_test.py
• core/domain/takeout_service_test.py
• core/domain/suggestion_services.py
• core/domain/suggestion_services_test.py
• core/domain/feedback_services_test.py
• core/controllers/feedback_updates_test.py
• core/controllers/creator_dashboard_test.py

the list of backed files that are using SuggestionEditStateContent domain are:

• core/domain/suggestion_registry.py
• core/domain/suggestion_registry_test.py
• core/domain/suggestion_services.py
• core/domain/suggestion_services_test.py
• core/controllers/suggestion_test.py
• core/controllers/suggestion.py
• core/controllers/feedback_updates.py

and in the frontend part will have to make changes to

• feedback update page
• creator dashboard page
• exploration editor page

After this I came to the conclusion that here we only need to delete GenerralSuggestionModel with suggestion_type
equals to feconf.SUGGESTION_TYPE_EDIT_STATE_CONTENT using beam job and then modify its associated files. After that will delete SuggestionEditStateContent domain and modify its associated files. As we are only deleting a subset of GenerralSuggestionModel so other parts of code using different type of GenerralSuggestionModel will work as usual.

[seanlip]

Thanks @masterboy376. Just to check though, did you see GeneralFeedbackThreadModel? It has a has_suggestion field. Does this relate to the SuggestionEditStateContent models in any way (will there be GeneralFeedbackThreadModel models that become invalid if a suggestion is deleted, or do we not currently use that functionality?)

[masterboy376]

Thanks @seanlip, I did notice it. This model itself does not directly depend on GenerralSuggestionModel with suggestion_type equal to feconf.SUGGESTION_TYPE_EDIT_STATE_CONTENT. However, there are some service and controller files that uses both of these models, specified above, so we have to make chhanges to these files to completely eliminate the use of feconf.SUGGESTION_TYPE_EDIT_STATE_CONTENT. Also has_suggestion field is a Boolean field that keeps record of whether the particular thread of particular entity_type has any suggestion or not. This suggestion can be on any type like translation suggestions or question suggestions.

[seanlip]

@masterboy376 I did a bit of digging and I think you are right -- we can probably delete these models and the associated code without issue.

From a bit of database archaeology, it looks like:

• Some explorations with accepted suggestions in the past are: 2EsrichXjAsX (suggestions were submitted at versions 39 and 65) and fUHjpTG2YT52 (suggestions were submitted at version 7)
• Some explorations with rejected suggestions are: rfX8jNkPnA-1 (submitted at version 34) and umPkwp0L1M0- (submitted at versions 100, 103, 121, 123; this also has one suggestion in "review" status submitte at version 85)
• Some explorations with "in review" suggestions are: lSNJ_1GnqVtC (submitted at version 8) and kal0aK5Keo08 (submitted at version 15).

I mention these because it would be good to mention them in the testing doc when you create your PR. We should run your job on a server environment and then explicitly visit these explorations (both the learner view and the Editor/Feedback tabs in the editor view) to make sure they still function correctly. Please make sure to include the testing doc when you submit the PR for review, and we can take it from there -- thanks!

[masterboy376]

@seanlip, just a question.
I have completed the beam job, registered and ran it on the local server and also wrote a test for it. So, now should I create a PR or should first I remove all the code that uses edit state content suggestion currently before submitting the PR. If later must be done then I wonder that how will beam job use feconf.SUGGESTION_TYPE_EDIT_STATE_CONTENT to fetch all such models.

[seanlip]

@masterboy376 Great question. I suggest creating a PR with the Beam job and cleaning up the datastore (as mentioned above, make sure to include a testing doc with your PR). Then make a separate PR that cleans up all the code and submit that only after we get confirmation that the Beam job succeeded.

One other note, Beam jobs that modify the datastore should be written in "audit" and "modify prod" versions (see existing examples in the codebase). Make sure your job is written that way too and that the audit job provides enough info to ensure that nothing goes amiss when the full job is run (it should probably just provide a list of things that will be deleted, but not actually do the deletions).