[Issue #1736] Adjust current opportunity summary logic to want null revision number #1743
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
chouinar
requested review from
acouch,
aplybeah and
SammySteiner
as code owners
rylew1
approved these changes
Apr 18, 2024
rylew1
reviewed
Apr 18, 2024
| if summary.is_forecast and summary.revision_number is None: | ||
| latest_forecasted_summary = summary | ||
| elif not summary.is_forecast and summary.revision_number is None: | ||
| latest_non_forecasted_summary = summary |
chouinar
added a commit
that referenced
this pull request
Apr 19, 2024
…ipt to generate them (#1773) ## Summary Fixes #1659 ### Time to review: __5 mins__ ## Changes proposed **this is temporary code and will be deleted after we run it - these are onetime scripts and are deliberately a bit hacky** Adds two scripts: * One that takes csvs extracts of the Oracle database tables and does transformations to make csvs we can import into our database * One that takes the output of the first script and uploads them into our database ## Context for reviewers As the transformation work is just starting, we want to unblock testing of the front-end UI by getting data into our dev environment. This approach is basically a hacky one-time shortcut to get data uploaded so that we have time to build the actual transformation process. Additionally, we using prod data so that it's realistic for testing purposes. The unit test I setup for the load script just exists to verify the tools work if the input dataset is on disk (as I did locally) or via S3 (as it will actually be run). ## Additional information Testing this requires downloading data from the real production database, which I think only I have access to at the moment. I did actually build this out / test it using that data locally. You can also use the test files I uploaded for the unit test to verify the behavior of the second part of the script. To run it: ``` poetry run python tests/util/convert_oracle_csvs_to_postgres.py --directory <whatever directory your csvs are in> poetry run flask task import-opportunity-csvs --input-folder <same directory as before> ``` Note this will not work at the moment due to needing a change from #1743
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #1736
Time to review: 3 mins
Changes proposed
Modify the set-current-opportunity script to determine the latest opportunity summary based on the revision number being null (rather than the max value)
Context for reviewers
My understanding of the revision number in the prior implementation was incorrect. We want to determine the latest revision of an opportunity summary based on the revision number, however the latest in the current Oracle system will always be null. The way the current system works is that the revision number is only added once it is moved to the history table. Because we effectively want to ignore all of the historical synopsis/forecast records, we can just check if the value is null.
Due to how the data is structured in the current system where the values are unique in the synopsis & forecast tables for a given opportunity, we know there will only ever be a single forecast / non-forecast that has this value set to null in the data.