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

[web] Move information about transactional system to the top of the storage page #1075

Merged
merged 5 commits into from
Mar 8, 2024

Conversation

ancorgs
Copy link
Contributor

@ancorgs ancorgs commented Mar 6, 2024

Problem

Agama shows a message in the storage screen if the product being installed uses a transactional Btrfs as root file-system (ie. read-only root subvolume and Btrfs snapshots as a mechanism to perform updates). But that message was displayed in the middle of the "Settings" section taking the place usually occupied by the Btrfs snapshots switch. Having it there didn't make much sense.

Old screen

Moreover, the sentence was a bit complicated and it was not so clear how the statement was related to other settings in the same page.

Solution

The message is now displayed at the top of the page and the wording has been slightly modified to, hopefully, explain why some of the usual configuration options (like disabling Btrfs snapshots or changing the file-system type for "/") are not available.

micro-reminder

If the system is not transactional, everything looks as before, as displayed in the following screenshots.

macro2

Testing

  • Tested manually
  • Updated unit tests

Note

This is broken into many small commits to ease collaboration during development. The plan is to squash when merging.

@ancorgs ancorgs force-pushed the transactional_on_top branch 2 times, most recently from 71d5465 to 124e308 Compare March 6, 2024 13:27
@coveralls
Copy link

coveralls commented Mar 6, 2024

Coverage Status

coverage: 74.427% (+0.03%) from 74.401%
when pulling 338b8e5 on ancorgs:transactional_on_top
into 987e8f7 on openSUSE:master.

@dgdavid

This comment was marked as outdated.

@dgdavid

This comment was marked as outdated.

@dgdavid

This comment was marked as resolved.

@dgdavid

This comment was marked as resolved.

web/src/components/storage/ProposalTransactionalInfo.jsx Outdated Show resolved Hide resolved
import { useProduct } from "~/context/product";

/**
* @typedef {import ("~/client/storage").ProposalManager.ProposalSettings} ProposalSettings
Copy link
Contributor

Choose a reason for hiding this comment

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

Note for future change: this is actually "not working". If you add the @ts-check comment directive to the file and then run the npm run check-types task it will complain because

src/components/storage/ProposalTransactionalInfo.jsx:34:42 - error TS2694: Namespace '"/src/client/storage"' has no exported member 'ProposalManager'.

34  * @typedef {import ("~/client/storage").ProposalManager.ProposalSettings} ProposalSettings
                            

And it is right. The ProposalManager class is actually not exported. I think there will be changes in the clients soon, not sure. But in any case, I'll move all the type definitions to the top of the file, avoiding to have them name spaced unless it make sense. @imobachgs, friendly ping.

web/src/components/storage/ProposalTransactionalInfo.jsx Outdated Show resolved Hide resolved
@ancorgs

This comment was marked as resolved.

@joseivanlopez

This comment was marked as resolved.

ancorgs and others added 3 commits March 7, 2024 11:50
Inspired in PF/Alert component, but much more simpler.
To make use of core/Reminder and return undefined as soon as possible.
Commit also add few changes to helper method.
@dgdavid
Copy link
Contributor

dgdavid commented Mar 7, 2024

I prefer the warning at the top because transactional products actually have implications in several sections. For example, if the system wouldn't be transactional, very likely there would be a "Btrfs Snapshots" toggle at the "Settings" section. Users who have already installed a non-transactional system may be wondering where it went.

Just for future reference, we had a short meeting to clarify this and as result a follow-up task was created. Find it at https://trello.com/c/ynFUHahV/353-agama-better-transactional-explanations-at-storage (internal link).

* @param {object} props
* @param {ProposalSettings} props.settings - Settings used for calculating a proposal.
*/
export default function ProposalTransactionalInfo({ settings }) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if this should from now on live at src/components/product/Transactional{Info|Reminder}.jsx, but it is something that I'd evaluate, if needed, in the context of https://trello.com/c/ynFUHahV/353-agama-better-transactional-explanations-at-storage.

Looks related to the product itself... but actually it checks the storage settings.

@ancorgs ancorgs changed the title [web] WIP: move transactional information [web] Move information about transactional system to the top of the storage page Mar 7, 2024
@ancorgs ancorgs marked this pull request as ready for review March 7, 2024 15:52
Copy link
Contributor

@joseivanlopez joseivanlopez left a comment

Choose a reason for hiding this comment

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

LGTM

@ancorgs ancorgs merged commit 49dfd5b into agama-project:master Mar 8, 2024
1 check passed
@imobachgs imobachgs mentioned this pull request May 17, 2024
imobachgs added a commit that referenced this pull request May 17, 2024
Prepare for releasing Agama 8. It includes the following pull requests:

* #884
* #886
* #914
* #918
* #956
* #957
* #958
* #959
* #960
* #961
* #962
* #963
* #964
* #965
* #966
* #969
* #970
* #976
* #977
* #978
* #979
* #980
* #981
* #983
* #984
* #985
* #986
* #988
* #991
* #992
* #995
* #996
* #997
* #999
* #1003
* #1004
* #1006
* #1007
* #1008
* #1009
* #1010
* #1011
* #1012
* #1014
* #1015
* #1016
* #1017
* #1020
* #1022
* #1023
* #1024
* #1025
* #1027
* #1028
* #1029
* #1030
* #1031
* #1032
* #1033
* #1034
* #1035
* #1036
* #1038
* #1039
* #1041
* #1042
* #1043
* #1045
* #1046
* #1047
* #1048
* #1052
* #1054
* #1056
* #1057
* #1060
* #1061
* #1062
* #1063
* #1064
* #1066
* #1067
* #1068
* #1069
* #1071
* #1072
* #1073
* #1074
* #1075
* #1079
* #1080
* #1081
* #1082
* #1085
* #1086
* #1087
* #1088
* #1089
* #1090
* #1091
* #1092
* #1093
* #1094
* #1095
* #1096
* #1097
* #1098
* #1099
* #1100
* #1102
* #1103
* #1104
* #1105
* #1106
* #1109
* #1110
* #1111
* #1112
* #1114
* #1116
* #1117
* #1118
* #1119
* #1120
* #1121
* #1122
* #1123
* #1125
* #1126
* #1127
* #1128
* #1129
* #1130
* #1131
* #1132
* #1133
* #1134
* #1135
* #1136
* #1138
* #1139
* #1140
* #1141
* #1142
* #1143
* #1144
* #1145
* #1146
* #1147
* #1148
* #1149
* #1151
* #1152
* #1153
* #1154
* #1155
* #1156
* #1157
* #1158
* #1160
* #1161
* #1162
* #1163
* #1164
* #1165
* #1166
* #1167
* #1168
* #1169
* #1170
* #1171
* #1172
* #1173
* #1174
* #1175
* #1177
* #1178
* #1180
* #1181
* #1182
* #1183
* #1184
* #1185
* #1187
* #1188
* #1189
* #1190
* #1191
* #1192
* #1193
* #1194
* #1195
* #1196
* #1198
* #1199
* #1200
* #1201
* #1203
* #1204
* #1205
* #1206
* #1207
* #1208
* #1209
* #1210
* #1211
* #1212
* #1213
* #1214
* #1215
* #1216
* #1217
* #1219
* #1220
* #1221
* #1222
* #1223
* #1224
* #1225
* #1226
* #1227
* #1229
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.

4 participants