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

Start using STI for decision model #16008

Merged
merged 8 commits into from Apr 22, 2024

Conversation

hellcp-work
Copy link
Contributor

@hellcp-work hellcp-work commented Apr 17, 2024

Expands the decision model into an STI model with two new classes.

Keep in mind this uses data migration, so if you try using this in your development setup, start off by running a rails data:migrate

Remaining tasks:

  • Fix the canned responses not working with this
  • Fix the events that fail in the tests

@github-actions github-actions bot added the Frontend Things related to the OBS RoR app label Apr 17, 2024
@hellcp-work hellcp-work force-pushed the refactor-decision-kind-2 branch 3 times, most recently from 4b58537 to 9bd7141 Compare April 18, 2024 08:29
@hellcp-work hellcp-work force-pushed the refactor-decision-kind-2 branch 2 times, most recently from 8aa582e to 27b0227 Compare April 18, 2024 09:45
@openSUSE openSUSE deleted a comment from github-actions bot Apr 18, 2024
@hellcp-work hellcp-work force-pushed the refactor-decision-kind-2 branch 2 times, most recently from 40dc496 to 738a06d Compare April 18, 2024 13:54
@saraycp
Copy link
Contributor

saraycp commented Apr 18, 2024

@hellcp-work The changes related to canned responses LGTM. Just fixing linters complaints is missing.

@saraycp saraycp mentioned this pull request Apr 18, 2024
@hellcp-work hellcp-work marked this pull request as ready for review April 18, 2024 16:11
@eduardoj
Copy link
Member

I would rename the last commit which includes all the changes to the specs from "Fix feature specs for canned responses with the new decision sti" to something more generic, like, for example, "Fix specs with the new decision sti".

@saraycp
Copy link
Contributor

saraycp commented Apr 19, 2024

With the proposed changes, we can get something like this:

=> #<DecisionFavored:0x00007f9001b9a648
[...]
 kind: "cleared",
 type: "DecisionFavored">

a newly created Decision where type is correct and kind is not. I know the reason is that we are no longer using kind and we are about to remove it, so we simply ignore it and the database takes the default value (cleared).

However, there is a gap between the code changes being applied and the data migration run, where someone can create a new decision and kind will be wrong. And then, when running the data migration we are going to take the wrong value and set it into the type.

I know we have little risk for this to happen, but who knows? We have to take OBS and IBS in mind. And the fix shouldn't be too difficult: write to both kind and type temporarily. It's also what strong_migrations suggests.

We've just done something similar in PR #16017, I could take care.

What do you think?

@hellcp-work
Copy link
Contributor Author

I know we have little risk for this to happen, but who knows? We have to take OBS and IBS in mind. And the fix shouldn't be too difficult: write to both kind and type temporarily. It's also what strong_migrations suggests.

We've just done something similar in PR #16017, I could take care.

What do you think?

It's not difficult to implement, we might as well do it

saraycp
saraycp previously approved these changes Apr 19, 2024
@saraycp saraycp dismissed their stale review April 19, 2024 13:01

The renaming of the spec commit message is missing

@danidoni
Copy link
Contributor

I would rename the last commit which includes all the changes to the specs from "Fix feature specs for canned responses with the new decision sti" to something more generic, like, for example, "Fix specs with the new decision sti".

Done

@eduardoj
Copy link
Member

I would rename the last commit which includes all the changes to the specs from "Fix feature specs for canned responses with the new decision sti" to something more generic, like, for example, "Fix specs with the new decision sti".

Done

I'm not fully convinced with "Make feature specs work with Decision STI". I would get rid of "feature". We are touching more specs than feature specs...

@danidoni
Copy link
Contributor

I'm not fully convinced with "Make feature specs work with Decision STI". I would get rid of "feature". We are touching more specs than feature specs...

Alright

@hellcp-work hellcp-work merged commit 6731487 into openSUSE:master Apr 22, 2024
21 of 22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Frontend Things related to the OBS RoR app
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants