Improvement to maintenance banner site setting

nirajkumar999 commented 1 month ago

Listing down the minor issues in maintenance banner site setting form and fixes to them

First Improvement :

Both settext and clearbanner buttons are of type brand. I changed clearbanner button to variant danger. Initial

After update

Second Improvement :

The inputbox was change from input to textarea. As here the admin would add some message hence it should look like textarea.

Third Improvement :

We have use localstorage to store maintenanceBannerId. Here in our code we fetch that value and pass to toast.dismiss function. As defined in documentation if the value passed to toast.dismiss is null that instead of raising any issue, it dismiss all the toast currently active which is not good for our application. Only required toast should be dismissed if its active else we should not called toast.dismiss. Now localstorage.getItem() returns null if there is no stored value corresponding to key passed to it. Hence this case should be handled.

Fourth Improvement :

Its good practice that after we extract the value from localstorage and if that value is no longer needed we should remove that key-value pair from localstorage. The same is updated in the code. After dismissing the toast we no longer need the maintenanceBannerId key-value in our localstorage.

Fifth Improvement :

Both the buttons settext and clearbanner are using spinner to show loading state but both the spinner are depending on same variable updateSiteSettingsAPI.isLoading. So both buttons goes in loading state at same time no matter which button is clicked. This is an undesired behavior so I have used two different variable for handling mutations on each button click. This way we can distinguish loading state for both the buttons.

  const updateSiteSettingsAPISetText = useUpdateSiteSettingsAPI();
  const updateSiteSettingsAPIClearText = useUpdateSiteSettingsAPI();

Sixth Improvement :

The issue i detected was, when some message is added to input box and settext button is clicked, we see a banner toast coming. But further when that message is updated, and settext is clicked another toast pops up with updated message. The catch is previous toast still persists. It gets removed only when the page is refreshed. So a better way would be to clear the banner toast if it exists and then display the updated banner toast. I have attached the image showing this unwanted behavior.

propose change 2

Initially my message was text1 so toast shows text1 on it. After updating it to text1 + text2 i get another toast with this update but previous one still exist on screen. In database the value updates correctly but over UI previous toast don't get removed without refreshing.

Seventh Improvement :

There is an performance issue on the buttons click for both settext and clearbanner. Even if no text is inputted still we can click both the buttons which leads to unnecessary PATCH request to my api. This behavior i tried to stop by sending PATCH request only if input box is not empty. So i added .required() validation using yup. Now settext wont lead to PATCH request if nothing is inputted. Also on empty input box clearbanner will not send any PATCH request.

Further optimization is done to both the buttons to send PATCH request only when there is actual change to message. I prevented api call if settetxt button is clicked unnecessary without any change to message. For this i have introduced a useRef() variable which tracks the value of banner message stored in db. This variable is modified accordingly when PATCH request to send.

The same optimizations can be extended for the LinksForm we have made for tnc, privacy policy and help center. I will update that in my another PR(#5891)

Eighth Improvement :

In useUpdateSiteSetting.jsx hook, the way error is handled is not proper.

onError: (error) => {
        if (error.response.data.errors.includes('Image MalwareDetected')) {
          toast.error(t('toast.error.malware_detected'));
        } else {
          handleError(error, t, toast);
        }
      },

On error we are calling handleError from FileValidationHelper.jsx which is basically designed to handle error assuming that there is issue with file upload.

export const handleError = (error, t, toast) => {
  switch (error.message) {
    case 'fileSizeTooLarge':
      toast.error(t('toast.error.file_size_too_large'));
      break;
    case 'fileTypeNotSupported':
      toast.error(t('toast.error.file_type_not_supported'));
      break;
    default:
      toast.error(t('toast.error.file_upload_error'));
  }
};

here in default case block we simply tell that there is file upload error. But this might not be apt for other cases. Eg - inside update action of admin/site_setting_controller.rb

site_setting = SiteSetting.joins(:setting)
                                    .find_by(
                                      provider: current_provider,
                                      setting: { name: params[:name] }
                                    )
return render_error status: :not_found unless site_setting

For any invalid setting name, site_setting sets to nil and we return not_found error. But this error is not handled inside useUpdateSiteSetting.jsx. Instead we are sending it to FileValidationHelper.jsx which treats this as upload error.

This case wont occur possibly but we should handle error judiciously. Hence a better and simple way would be to change the default case error message inside FileValidationHelper,jsx to a generic error message. This will cover all the cases.


export const handleError = (error, t, toast) => {
  switch (error.message) {
    case 'fileSizeTooLarge':
      toast.error(t('toast.error.file_size_too_large'));
      break;
    case 'fileTypeNotSupported':
      toast.error(t('toast.error.file_type_not_supported'));
      break;
    default:
      toast.error(t('toast.error.problem_completing_action'));
  }
};

Thank You.

Further Suggestions are welcomed