search

se-edu / addressbook-level3

:ab::three: Address Book sample application (Level 3)
https://se-education.org/addressbook-level3
MIT License
28 stars 426 forks source link

[#73] no write access fail gracefully #172

Closed Eclipse-Dominator closed 1 year ago

Eclipse-Dominator commented 1 year ago

Fixes #73 Continued from #77

When there is insufficient permission to write data to the files, an AccessDeniedException will appear in the error message. This is not as informative as the error message is the same for both AccessDeniedException and IOException. Thus let's make the error message more descriptive for both exceptions.

Besides this, the same error message (Invalid Command: <command text> is logged whenever a CommandException or ParseException occurs. However, such errors can be due to IOExceptions which is not related to the invalid command.

Therefore, there is also a need to improve the accuracy of the error message by changing it to more accurately describe the failure.

canihasreview[bot] commented 1 year ago

Click here to submit a new iteration when this PR is ready for review.

See this repository's contribution guide for more information.

canihasreview[bot] commented 1 year ago

v1

@Eclipse-Dominator submitted v1 for review.

(:books: Archive)

Checkout this PR version locally ```sh git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/172/1/head:BRANCHNAME ``` where `BRANCHNAME` is the name of the local branch you wish to fetch this PR to.
canihasreview[bot] commented 1 year ago

v2

@Eclipse-Dominator submitted v2 for review.

(:books: Archive) (:chart_with_upwards_trend: Interdiff between v1 and v2) (:chart_with_upwards_trend: Range-Diff between v1 and v2)

Checkout this PR version locally ```sh git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/172/2/head:BRANCHNAME ``` where `BRANCHNAME` is the name of the local branch you wish to fetch this PR to.
Eclipse-Dominator commented 1 year ago

Apologies for all the changes, I realised I forgot to include a line spacing between the commit header and the commit body

damithc commented 1 year ago

There is an issue with the exit command where exit will attempt to save all file before exiting and due to the permission error, it will prevent the program from exiting.

hmm... there is no real need to save data upon exit. Perhaps we can skip the data saving if the command is the exit command?

Eclipse-Dominator commented 1 year ago

Hmm I don't think that there is a very clean way to identify specific command without hardcoding certain checks. 

@Override
    public CommandResult execute(String commandText) throws CommandException, ParseException {
        logger.info("----------------[USER COMMAND][" + commandText + "]");

        CommandResult commandResult;
        Command command = addressBookParser.parseCommand(commandText);
        commandResult = command.execute(model);

        try {
            storage.saveAddressBook(model.getAddressBook());
        } catch (IOException ioe) {
            throw new CommandException(FILE_OPS_ERROR_MESSAGE + ioe, ioe);
        }

        return commandResult;
    }
    private CommandResult executeCommand(String commandText) throws CommandException, ParseException {
        try {
            CommandResult commandResult = logic.execute(commandText);
            logger.info("Result: " + commandResult.getFeedbackToUser());
            resultDisplay.setFeedbackToUser(commandResult.getFeedbackToUser());

            if (commandResult.isShowHelp()) {
                handleHelp();
            }

            if (commandResult.isExit()) {
                handleExit();
            }

            return commandResult;
        } catch (CommandException | ParseException e) {
            logger.info("Invalid command: " + commandText);
            resultDisplay.setFeedbackToUser(e.getMessage());
            throw e;
        }
    }

Saving of addressbook is done in execute itself and only afterwards verification of whether something is help or exit is done.

I was thinking of allowing execute to throw a custom exception to represent critical error where the program need to exit instead of a CommandException

        try {
            storage.saveAddressBook(model.getAddressBook());
        } catch (IOException ioe) {
            throw new CriticalException(FILE_OPS_ERROR_MESSAGE + ioe, ioe); // <-- 
        }

Then specifically catching it in MainWindow.java

        catch (CriticalException e) {
            handleCriticalError(e); // <-- spawns a popup that exits program after closing. 
        }

Alternatively, we can also store the exception in CommandResult instead of catching it later on. In a sense we are decoupling the save exception from the parse exceptions. However, both of these method could be an overkill to handle permission denied error.

codecov-commenter commented 1 year ago

Codecov Report

Merging #172 (3f69739) into master (c9c9512) will increase coverage by 0.04%. The diff coverage is 75.00%.

@@             Coverage Diff              @@
##             master     #172      +/-   ##
============================================
+ Coverage     73.96%   74.00%   +0.04%     
  Complexity      420      420              
============================================
  Files            71       71              
  Lines          1279     1281       +2     
  Branches        126      126              
============================================
+ Hits            946      948       +2     
  Misses          301      301              
  Partials         32       32
Impacted Files Coverage Δ
src/main/java/seedu/address/ui/MainWindow.java 0.00% <0.00%> (ø)
...rc/main/java/seedu/address/logic/LogicManager.java 77.27% <100.00%> (+2.27%) :arrow_up:

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

damithc commented 1 year ago

As this is a very rare case, we should try a solution that minimizes changes to the code. So, approach 3 should be fine. Do you think this PR can be just one commit?

Eclipse-Dominator commented 1 year ago

I think 2 commit is probably more suited here?

  1. Decoupling IOException and AccessDeniedException
  2. Decoupling ParseException and CommandException

As for the commit body, I am thinking of 

  1. 
Better error message when file have no write-access

When the data file has no write access, a message containing. a raw exception is displayed to the user with no specific instructions on handling such errors.

Let's

All errors related to command is logged as "invalid command". Let's be more specific by decoupling ParseExceptions and CommandExceptions to enable an more accurate logging of the errors.

damithc commented 1 year ago

I'm not sure if the second commit adds enough value. While the same prefix is used in both cases, the error message itself should tell us what exactly happened, right? Don't forget that commit message subject should be in imperative mood.

Eclipse-Dominator commented 1 year ago

I have updated the commit to the following:

Improve error message if no write permission

Insufficient permission to read the data file will show
an AccessDeniedException as error message.
AB3 should be able to fail gracefully with instructions
to close the application upon failure.

Let's
- make the error message more user friendly by informing
the user that there are insufficient permissions for the file.
- Decouple AccessDeniedException from IOException messages.
- Decouple ParseException and CommandException log messages.
canihasreview[bot] commented 1 year ago

v3

@Eclipse-Dominator submitted v3 for review.

(:books: Archive) (:chart_with_upwards_trend: Interdiff between v2 and v3) (:chart_with_upwards_trend: Range-Diff between v2 and v3)

Checkout this PR version locally ```sh git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/172/3/head:BRANCHNAME ``` where `BRANCHNAME` is the name of the local branch you wish to fetch this PR to.
canihasreview[bot] commented 1 year ago

v4

@Eclipse-Dominator submitted v4 for review.

(:books: Archive) (:chart_with_upwards_trend: Interdiff between v3 and v4) (:chart_with_upwards_trend: Range-Diff between v3 and v4)

Checkout this PR version locally ```sh git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/172/4/head:BRANCHNAME ``` where `BRANCHNAME` is the name of the local branch you wish to fetch this PR to.
canihasreview[bot] commented 1 year ago

v5

@Eclipse-Dominator submitted v5 for review.

(:books: Archive) (:chart_with_upwards_trend: Interdiff between v4 and v5) (:chart_with_upwards_trend: Range-Diff between v4 and v5)

Checkout this PR version locally ```sh git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/172/5/head:BRANCHNAME ``` where `BRANCHNAME` is the name of the local branch you wish to fetch this PR to.
canihasreview[bot] commented 1 year ago

v6

@Eclipse-Dominator submitted v6 for review.

(:books: Archive) (:chart_with_upwards_trend: Interdiff between v5 and v6) (:chart_with_upwards_trend: Range-Diff between v5 and v6)

Checkout this PR version locally ```sh git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/172/6/head:BRANCHNAME ``` where `BRANCHNAME` is the name of the local branch you wish to fetch this PR to.
canihasreview[bot] commented 1 year ago

v7

@Eclipse-Dominator submitted v7 for review.

(:books: Archive) (:chart_with_upwards_trend: Interdiff between v6 and v7) (:chart_with_upwards_trend: Range-Diff between v6 and v7)

Checkout this PR version locally ```sh git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/172/7/head:BRANCHNAME ``` where `BRANCHNAME` is the name of the local branch you wish to fetch this PR to.
canihasreview[bot] commented 1 year ago

v8

@Eclipse-Dominator submitted v8 for review.

(:books: Archive) (:chart_with_upwards_trend: Interdiff between v7 and v8) (:chart_with_upwards_trend: Range-Diff between v7 and v8)

Checkout this PR version locally ```sh git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/172/8/head:BRANCHNAME ``` where `BRANCHNAME` is the name of the local branch you wish to fetch this PR to.
canihasreview[bot] commented 1 year ago

v9

@Eclipse-Dominator submitted v9 for review.

(:books: Archive) (:chart_with_upwards_trend: Interdiff between v8 and v9) (:chart_with_upwards_trend: Range-Diff between v8 and v9)

Checkout this PR version locally ```sh git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/172/9/head:BRANCHNAME ``` where `BRANCHNAME` is the name of the local branch you wish to fetch this PR to.
canihasreview[bot] commented 1 year ago

v10

@Eclipse-Dominator submitted v10 for review.

(:books: Archive) (:chart_with_upwards_trend: Interdiff between v9 and v10) (:chart_with_upwards_trend: Range-Diff between v9 and v10)

Checkout this PR version locally ```sh git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/172/10/head:BRANCHNAME ``` where `BRANCHNAME` is the name of the local branch you wish to fetch this PR to.
canihasreview[bot] commented 1 year ago

v11

@Eclipse-Dominator submitted v11 for review.

(:books: Archive) (:chart_with_upwards_trend: Interdiff between v10 and v11) (:chart_with_upwards_trend: Range-Diff between v10 and v11)

Checkout this PR version locally ```sh git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/172/11/head:BRANCHNAME ``` where `BRANCHNAME` is the name of the local branch you wish to fetch this PR to.
damithc commented 1 year ago

@se-edu/tech-team-level1 for your review ...

damithc commented 1 year ago

Just a minor sugguestion on the user interface. Currently, the exception messages are too long, causing some readability issues. It may be more user-friendly to insert line breaks into these messages to enhance their legibility.

Fair point @SPWwj Probably OK in this case because the second sentence follows directly from the first. We don't want to give the impression that it is a separate problem. In fact I wonder if we should phrase it as ...book.json due to insufficient ... 🤔

SPWwj commented 1 year ago

Just a minor sugguestion on the user interface. Currently, the exception messages are too long, causing some readability issues. It may be more user-friendly to insert line breaks into these messages to enhance their legibility.

Fair point @SPWwj Probably OK in this case because the second sentence follows directly from the first. We don't want to give the impression that it is a separate problem. In fact I wonder if we should phrase it as ...book.json due to insufficient ... 🤔

Agreed, this completes the sentence, enhancing clarity and precision by providing a reason. In circumstances with multiple issues, it ensures that no ambiguity arises from multiple sentences.

canihasreview[bot] commented 1 year ago

v12

@Eclipse-Dominator submitted v12 for review.

(:books: Archive) (:chart_with_upwards_trend: Interdiff between v11 and v12) (:chart_with_upwards_trend: Range-Diff between v11 and v12)

Checkout this PR version locally ```sh git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/172/12/head:BRANCHNAME ``` where `BRANCHNAME` is the name of the local branch you wish to fetch this PR to.
canihasreview[bot] commented 1 year ago

v13

@Eclipse-Dominator submitted v13 for review.

(:books: Archive) (:chart_with_upwards_trend: Interdiff between v12 and v13) (:chart_with_upwards_trend: Range-Diff between v12 and v13)

Checkout this PR version locally ```sh git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/172/13/head:BRANCHNAME ``` where `BRANCHNAME` is the name of the local branch you wish to fetch this PR to.
canihasreview[bot] commented 1 year ago

v14

@Eclipse-Dominator submitted v14 for review.

(:books: Archive) (:chart_with_upwards_trend: Interdiff between v13 and v14) (:chart_with_upwards_trend: Range-Diff between v13 and v14)

Checkout this PR version locally ```sh git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/172/14/head:BRANCHNAME ``` where `BRANCHNAME` is the name of the local branch you wish to fetch this PR to.
damithc commented 1 year ago

@chia-yh @hansstanley @SPWwj Can we have another review, as the code has changed since your last review?

canihasreview[bot] commented 1 year ago

v15

@Eclipse-Dominator submitted v15 for review.

(:books: Archive) (:chart_with_upwards_trend: Interdiff between v14 and v15) (:chart_with_upwards_trend: Range-Diff between v14 and v15)

Checkout this PR version locally ```sh git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/172/15/head:BRANCHNAME ``` where `BRANCHNAME` is the name of the local branch you wish to fetch this PR to.
canihasreview[bot] commented 1 year ago

v16

@Eclipse-Dominator submitted v16 for review.

(:books: Archive) (:chart_with_upwards_trend: Interdiff between v15 and v16) (:chart_with_upwards_trend: Range-Diff between v15 and v16)

Checkout this PR version locally ```sh git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/172/16/head:BRANCHNAME ``` where `BRANCHNAME` is the name of the local branch you wish to fetch this PR to.
damithc commented 1 year ago

Thanks for these improvements, @Eclipse-Dominator