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.

Sign up for GitHub

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

Jump to bottom

[RB] Fix bmi DB error #8673

Merged
merged 2 commits into from
May 2, 2023
Merged

[RB] Fix bmi DB error #8673

merged 2 commits into from
May 2, 2023

Conversation

laemtl
Copy link
Contributor

@laemtl laemtl commented Apr 25, 2023 •
edited
Loading

An inconsistency between the linst file and the sql schema in the enum field unit_classification trigger a 500 error. Fixed by updating the enum with the missing value in the SQL schema.

Fixes #8578

@laemtl laemtl closed this Apr 25, 2023
@laemtl laemtl reopened this Apr 25, 2023
@laemtl laemtl changed the base branch from main to 25.0-release April 25, 2023 18:16
@laemtl laemtl added this to the 25.0.0 milestone Apr 25, 2023
@laemtl
Copy link
Contributor Author

laemtl commented Apr 25, 2023 •
edited
Loading

@zaliqarosli I apply the changes in the test VM to help you testing.

@laemtl laemtl requested a review from zaliqarosli April 25, 2023 18:20
zaliqarosli
zaliqarosli requested changes Apr 25, 2023
View reviewed changes
raisinbread/instruments/bmi.linst Outdated Show resolved Hide resolved
raisinbread/instruments/instrument_sql/bmi.sql Outdated Show resolved Hide resolved
raisinbread/instruments/bmi.linst Outdated Show resolved Hide resolved
@laemtl laemtl requested a review from zaliqarosli April 26, 2023 16:20
@laemtl
Copy link
Contributor Author

laemtl commented Apr 26, 2023

@zaliqarosli Ready for second review

zaliqarosli
zaliqarosli requested changes Apr 26, 2023
View reviewed changes
Copy link
Contributor

@zaliqarosli zaliqarosli left a comment

Choose a reason for hiding this comment

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

i think changes to raisinbread/instruments/bmi.linst can be removed completely cz its just a permissions change which i think is unnecessary

zaliqarosli
zaliqarosli reviewed Apr 26, 2023
View reviewed changes
Copy link
Contributor

@zaliqarosli zaliqarosli left a comment

Choose a reason for hiding this comment

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

PR looks good, i no longer have the issue reported in #8578. I re-ran raisinbread and changes here look good, no 500 error.

I am getting a 400 though that seems to be coming from the React upgrade regression PR? #8651

Screenshot 2023-04-26 at 1 13 59 PM

@laemtl laemtl requested a review from zaliqarosli April 26, 2023 17:23
@laemtl
Copy link
Contributor Author

laemtl commented Apr 26, 2023 •
edited
Loading

Hum, let me check the new error. I fixed the perm change, thanks for the hint :)

@laemtl
Copy link
Contributor Author

laemtl commented Apr 26, 2023

New error fixed in #8681

@laemtl laemtl mentioned this pull request Apr 26, 2023
Merged
@zaliqarosli zaliqarosli added the Passed manual tests PR has been successfully tested by at least one peer label Apr 26, 2023
@driusan driusan merged commit bc3e4a8 into aces:25.0-release May 2, 2023
driusan pushed a commit that referenced this pull request May 11, 2023
@laemtl
[Instrument] ControlpanelDeleteInstrumentData error (#8681)
06d27dc 
Fix clear instrument button regression introduced by #7018 and reported in PR #8673 

Fixes #7065, Fixes #8580
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zaliqarosli zaliqarosli zaliqarosli approved these changes

Assignees

@zaliqarosli zaliqarosli

Labels
25.0.0 - Bugs Passed manual tests PR has been successfully tested by at least one peer
Projects
None yet
Milestone
25.0.0
Development

Successfully merging this pull request may close these issues.
4 participants
@laemtl @zaliqarosli @driusan @cmadjar