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 #10274: Create global initial version constant and use it for initializing all the versioned entities #20301

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

Patrick-Haselof
Copy link

@Patrick-Haselof Patrick-Haselof commented May 12, 2024

Overview

  1. This PR fixes or fixes part of Create global initial version constant and use it for initializing all the versioned entities #10274 .
  2. This PR does the following: According to the Issue "Currently, the initial value for the version number is hardcoded in the domain layer for all the entities which make it open for any entity to add incorrect initial version number", following by suggesting the creation of a Global Variable to act as a consultable constant that all files that require the initial version number can access. It was also pointed that there may be more files that needed refactoring, so after doing what was suggested and created a constant called INITIAL_ENTITY_VERSION in feconf.py, I tried to find more files that needed to be modified. The files controllers and domain have been fully searched:

Modified files:

  • exp_domain.py
  • topic_domain.py
  • collection_domain.py
  • exp_fetchers_test.py
  • question_domain.py
  • story_domain.py
  • subtopic_page_domain.py

Found that needed refactoring but needs to be looked into/Uncertain of how to properly modify:

  • skill_domain.py
  • stats_domain.py

This Issue fix is pure refactoring and no logic has been modified or added

Essential Checklist

Please follow the instructions for making a code change.

  • I have linked the issue that this PR fixes in the "Development" section of the sidebar.
  • I have checked the "Files Changed" tab and confirmed that the changes are what I want to make.
  • I have written tests for my code.
  • The PR title starts with "Fix #bugnum: " or "Fix part of #bugnum: ...", followed by a short, clear summary of the changes.
  • I have assigned the correct reviewers to this PR (or will leave a comment with the phrase "@{{reviewer_username}} PTAL" if I can't assign them directly).

Proof that changes are correct

This is a simple refactoring work. Since the code logic has not been modified there is no proof.

PR Pointers

@Patrick-Haselof Patrick-Haselof requested a review from a team as a code owner May 12, 2024 22:37
Copy link

oppiabot bot commented May 12, 2024

Hi @Patrick-Haselof, can you complete the following:

  1. The body of this PR is missing the overview section, please update it to include the overview.
    Thanks!

Copy link

oppiabot bot commented May 12, 2024

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

Copy link

Hi! @Patrick-Haselof Welcome to Oppia! Could you please follow the instructions here and sign the CLA Sheet to get started? You'll need to do this before we can accept your PR. Thanks!

@Patrick-Haselof
Copy link
Author

This was an accident, I meant to push to my Fork

Copy link

oppiabot bot commented May 12, 2024

Hi @Patrick-Haselof, can you complete the following:

  1. The body of this PR is missing the overview section, please update it to include the overview.
    Thanks!

Copy link

Hi! @Patrick-Haselof Welcome to Oppia! Could you please follow the instructions here and sign the CLA Sheet to get started? You'll need to do this before we can accept your PR. Thanks!

@Patrick-Haselof
Copy link
Author

Patrick-Haselof commented May 12, 2024

This PR is trying to merge from my Branch instead of my Fork. I tried editing and even creating a new PR, didn't manage to do it, so for now I'll leave this open.

@lkbhitesh07
Copy link
Member

Hey @Patrick-Haselof, thanks for the changes. I would suggest to follow this comment mentioned on the issue and then make the changes to this PR, this is the pattern we are currently following.
Thanks

@lkbhitesh07 lkbhitesh07 removed their assignment May 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants