[OSDEV-1429] Block list uploading during release process by Innavin369 · Pull Request #415 · opensupplyhub/open-supply-hub

Innavin369 · 2024-11-13T16:48:25Z

OSDEV-1429 The list upload switcher has been created to disable the Submit button on the List Contribute page through the Switch page in the Django admin panel during the release process and implemented a check on the list upload endpoint.

barecheck · 2024-11-13T16:50:54Z

React App | Jest test suite - Code coverage report

Total: 27.8%

Your code coverage diff: 0.87% ▴

✅ All code changes are covered

barecheck · 2024-11-13T16:55:41Z

Dedupe Hub App | Unittest test suite - Code coverage report

Total: 56.14%

Your code coverage diff: 0.00% ▴

✅ All code changes are covered

VadimKovalenkoSNF

Looks ok, but require minor changes + address coderabbitai suggestions.

src/django/api/exceptions.py

coderabbitai

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (3)

src/django/api/tests/test_facility_list_create.py (3)
9-9: Remove unused import

The re module is imported but never used in the code.
-import re
🧰 Tools

🪛 Ruff (0.7.0)

9-9: re imported but unused

Remove unused import: re

(F401)

255-257: Improve string formatting

The multi-line error message string could be formatted more cleanly using parentheses instead of line continuation characters.
-        expected = ["Open Supply Hub is undergoing maintenance and not \
-                    accepting new data at the moment. Please try again in a \
-                    few minutes."]
+        expected = [
+            "Open Supply Hub is undergoing maintenance and not "
+            "accepting new data at the moment. Please try again in a "
+            "few minutes."
+        ]
251-252: Consider a more specific test name

The current test name could be more explicit about the expected behavior when uploads are disabled.
-    def test_no_upload_when_disable_list_uploading_switch_active(self):
+    def test_returns_503_when_list_uploading_is_disabled(self):

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 4f2acfc and ed5a470.

📒 Files selected for processing (2)

src/django/api/exceptions.py (1 hunks)
src/django/api/tests/test_facility_list_create.py (2 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

src/django/api/exceptions.py

🧰 Additional context used

🪛 Ruff (0.7.0)

src/django/api/tests/test_facility_list_create.py

9-9: re imported but unused

Remove unused import: re

(F401)

🔇 Additional comments (1)

src/django/api/tests/test_facility_list_create.py (1)

253-280: LGTM! Test implementation is thorough

The test comprehensively verifies:

Correct HTTP status code for maintenance mode
Proper error message format
No side effects on database state (list and source counts remain unchanged)

VadimKovalenkoSNF

Approved.

coderabbitai

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (2)

src/django/api/tests/test_facility_list_create.py (2)
9-9: Remove unused import

The re module is imported but not used in the code. It appears to be a leftover from a previous implementation that used regex for string comparison.
-import re
🧰 Tools

🪛 Ruff (0.7.0)

9-9: re imported but unused

Remove unused import: re

(F401)

255-257: Improve error message string formatting

The multi-line string could be formatted more cleanly to avoid unnecessary whitespace in the message.
-        expected = ["Open Supply Hub is undergoing maintenance and not \
-                    accepting new data at the moment. Please try again in a \
-                    few minutes."]
+        expected = ["Open Supply Hub is undergoing maintenance and not "
+                   "accepting new data at the moment. Please try again in a "
+                   "few minutes."]

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between ed5a470 and 2100446.

📒 Files selected for processing (1)

src/django/api/tests/test_facility_list_create.py (2 hunks)

🧰 Additional context used

🪛 Ruff (0.7.0)

src/django/api/tests/test_facility_list_create.py

9-9: re imported but unused

Remove unused import: re

(F401)

🔇 Additional comments (1)

src/django/api/tests/test_facility_list_create.py (1)

251-279: LGTM! Well-structured test implementation.

The test comprehensively verifies the list upload blocking functionality during maintenance:

Simulates feature flag state
Verifies correct error response
Ensures no side effects on database

VadimKovalenkoSNF

Minor request required.

src/django/api/migrations/0161_create_disable_list_uploading_switch.py

src/django/api/views/facility/facility_list_view_set.py

src/react/src/components/ContributeForm.jsx

mazursasha1990 · 2024-11-22T10:28:24Z

Please fix the linter error.

coderabbitai

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (2)

src/react/src/components/ContributeForm.jsx (2)

57-67: Consider using theme variables for colors and sizes.

The tooltip styling is well-implemented, but consider using theme variables for colors and sizes to maintain consistency across the application.

 const StyledTooltip = withStyles(theme => ({
     tooltip: {
-        color: 'rgba(0, 0, 0, 0.8)',
+        color: theme.palette.text.primary,
         fontSize: theme.typography.fontSize,
         backgroundColor: 'white',
-        border: 'solid rgba(0, 0, 0, 0.25)',
+        border: `solid ${theme.palette.divider}`,
         borderRadius: theme.shape.borderRadius,
         padding: theme.spacing(1),
         lineHeight: '1',
     },
 }))(Tooltip);

181-208: Consider optimizing the maintenance mode button implementation.

While the implementation is functional, it could be simplified:

The div wrapper could be removed by applying the inline style directly to the Button
Consider using 'top' placement for the tooltip for better visibility on mobile devices

                     <FeatureFlag
                         flag={DISABLE_LIST_UPLOADING}
                         alternative={
                             <Button
                                 onClick={handleUploadList}
                                 disabled={submitButtonIsDisabled}
                                 text="SUBMIT"
                                 variant="contained"
                                 disableRipple
                             />
                         }
                     >
                         <StyledTooltip
                             title={MAINTENANCE_MESSAGE}
-                            placement="right"
+                            placement="top"
                         >
-                            <div style={contributeFormStyles.inline}>
                                 <Button
                                     disabled
                                     text="SUBMIT"
                                     variant="contained"
                                     disableRipple
                                     onClick={() => {}}
                                     aria-label="Submit button disabled during maintenance"
+                                    style={contributeFormStyles.inline}
                                 />
-                            </div>
                         </StyledTooltip>
                     </FeatureFlag>

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 226324d and b9e94be.

📒 Files selected for processing (3)

src/django/api/tests/test_facility_list_create.py (2 hunks)
src/django/api/views/facility/facility_list_view_set.py (3 hunks)
src/react/src/components/ContributeForm.jsx (7 hunks)

🚧 Files skipped from review as they are similar to previous changes (2)

src/django/api/tests/test_facility_list_create.py
src/django/api/views/facility/facility_list_view_set.py

🔇 Additional comments (3)

src/react/src/components/ContributeForm.jsx (3)

15-16: LGTM: Import statements are well-organized.

The new imports are logically grouped and necessary for implementing the list upload switcher feature.

Also applies to: 19-19, 32-37

81-83: LGTM: Style object follows immutability pattern.

The inline style is properly implemented and frozen for immutability.

217-217: LGTM: Props changes follow React best practices.

Making uploadList optional with a default no-op function is a good practice for handling the maintenance mode case.

Also applies to: 231-231

sonarqubecloud · 2024-11-22T11:37:14Z

Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

coderabbitai

Actionable comments posted: 2

🧹 Outside diff range and nitpick comments (2)

src/react/src/components/Button.jsx (1)
Line range hint 5-39: Add JSDoc documentation for the Button component

Since this component is being modified to support new use cases around disabled states, it would be helpful to add documentation explaining its usage patterns and props.

Add JSDoc documentation above the component:
+/**
+ * A wrapper around Material-UI's Button component with custom styling.
+ * 
+ * @component
+ * @example
+ * // Basic usage
+ * <Button onClick={handleClick} text="Submit" />
+ * 
+ * // Disabled state
+ * <Button onClick={handleClick} text="Submit" disabled={true} />
+ * 
+ * @property {Function} [onClick] - Click handler function
+ * @property {string} text - Button label text
+ * @property {Function} [Icon] - Optional icon component to render
+ * @property {boolean} [disabled=false] - Whether the button is disabled
+ * @property {Object} [style] - Additional inline styles
+ */
 class Button extends PureComponent {
src/react/src/components/ContributeForm.jsx (1)
57-67: Consider using theme variables for better maintainability.

The tooltip styling is well-implemented, but hardcoded values could be moved to theme variables for better maintainability.

Consider refactoring to use theme variables:
 const StyledTooltip = withStyles(theme => ({
     tooltip: {
-        color: 'rgba(0, 0, 0, 0.8)',
-        fontSize: '0.875rem',
+        color: theme.palette.text.primary,
+        fontSize: theme.typography.fontSize,
         backgroundColor: 'white',
-        border: 'solid rgba(0, 0, 0, 0.25)',
+        border: `solid ${theme.palette.divider}`,
         borderRadius: '10px',
         padding: '10px',
         lineHeight: '1',
     },
 }))(Tooltip);

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between b9e94be and f702690.

📒 Files selected for processing (2)

src/react/src/components/Button.jsx (2 hunks)
src/react/src/components/ContributeForm.jsx (5 hunks)

🔇 Additional comments (3)

src/react/src/components/ContributeForm.jsx (3)

15-16: LGTM: Import statements are well-organized.

The new imports are properly structured and necessary for implementing the list upload switcher feature.

Also applies to: 19-19, 32-37

81-83: LGTM: Style object follows existing patterns.

The new inline style is properly implemented using Object.freeze, maintaining consistency with the existing codebase.

176-207: LGTM: Feature flag implementation is well-structured and accessible.

The implementation:

Correctly handles both enabled and disabled states
Includes proper accessibility attributes
Maintains clean code structure
Retains empty onClick handler to prevent console warnings (as discussed in previous reviews)

src/react/src/components/Button.jsx

src/react/src/components/ContributeForm.jsx

VadimKovalenkoSNF

Approved.

vladsha-dev

LGTM

Inessa Druzhkova added 4 commits November 13, 2024 14:58

created flag, added disabled btn to UI

831ac70

added migration

3f4ba57

added task description

84cb2d6

added flag on BE side

ea765f9

Innavin369 marked this pull request as draft November 13, 2024 16:48