refactor: error messages reformatted and moved into own files (#1731)

[fn-faisal]

New Pull Request Checklist

• [x] I am not disclosing a vulnerability.
• [x] I am creating this PR in reference to an issue.

Issue Description

Message error for required fields #1731

Related issue: Issue link

Approach

This is a PR regarding the below issue. The formatting has been added to the server. I'll update the existing PR for the dashboard to exclude formatting of response string from dashboard. https://github.com/parse-community/parse-dashboard/issues/1731#issuecomment-865070353

[mtrezza]

Not sure about this PR, see https://github.com/parse-community/parse-dashboard/issues/1731#issuecomment-868657186.

[fn-faisal]

I have updated this PR to refactor error messages from server into their own files. This way, the error messages are easier to update. I have also updated the error message for the required fields.

[fn-faisal]

@mtrezza For this one, I refactored the messages in the a separate file and refactored the messages. I wasn't too sure about the changes required for this issue but from what I understood, I refactored the message strings in their own file and referenced them in the error responses. I didn't see a support for locale strings and wasn't sure if multiple locales were supported so I made a new file for locale string.

[mtrezza]

That's certainly a nice consolidation!

• There are still some error messages that should begin uppercase, I think, for example 'bad or missing username'
• Could you rename the message.js path to be more specific, for example /Errors/messages.js? These should actually go into the Parse JS SDK Parse.Error object, but that is so complicated at the moment that we can leave this here for now. The errors will hopefully go into their own repo in the future, to be able to reference them from all other SDKs as well.
• For the tests to pass, they should also use the error references.

[codecov[bot]]

Codecov Report

Merging #7446 (0d6b06a) into alpha (826aa79) will decrease coverage by 0.01%. The diff coverage is 78.57%.

@@            Coverage Diff             @@
##            alpha    #7446      +/-   ##
==========================================
- Coverage   93.98%   93.96%   -0.02%     
==========================================
  Files         183      184       +1     
  Lines       13655    13695      +40     
==========================================
+ Hits        12833    12868      +35     
- Misses        822      827       +5     

Impacted Files	Coverage Δ
src/GraphQL/loaders/usersMutations.js	91.39% <25.00%> (+0.09%)	:arrow_up:
src/Adapters/Storage/Mongo/MongoTransform.js	88.12% <62.50%> (+0.01%)	:arrow_up:
src/RestWrite.js	94.20% <71.42%> (ø)
src/Routers/UsersRouter.js	94.41% <82.35%> (+0.03%)	:arrow_up:
src/Adapters/Storage/Mongo/MongoStorageAdapter.js	93.37% <83.33%> (+0.01%)	:arrow_up:
src/Errors/message.js	84.84% <84.84%> (ø)
...dapters/Storage/Postgres/PostgresStorageAdapter.js	95.46% <92.85%> (+<0.01%)	:arrow_up:
src/AccountLockout.js	97.61% <100.00%> (+0.05%)	:arrow_up:
src/RestQuery.js	95.71% <100.00%> (+<0.01%)	:arrow_up:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 826aa79...0d6b06a. Read the comment docs.

[mtrezza]

@fn-faisal Is this ready for review?

[fn-faisal]

@mtrezza Yes, it's ready for review.

[parse-github-assistant[bot]]

Thanks for opening this pull request!

• ❌ Please edit your post and use the provided template when creating a new pull request. This helps everyone to understand your post better and asks for essential information to quicker review the pull request.

[mtrezza]

• Could you add a changelog entry?
• I see that some error messages are ending with a period (.), others are not. Should all end with a period or none? I would think that all should end with period, because it makes the end of the message and it allows to concat another message, if that should be necessary in any case. What do you think?

[fn-faisal]

@mtrezza The error messages could be used in Popups, Alerts or sequential toasts and they are single lines. In my opinion a . at the end looks off in some of the uses cases. I have removed the . but can add it back in if you believe the . should be added at the end of the messages. I have added an entry to the CHANGELOG.

[mtrezza]

Can you give an example when it would be odd to see a dot at the end of an error message?

I think the question is how we expect these messages to be formed. If they are formed like a sentence, possibly containing other punctuation like comma, then a period at the end would be appropriate. If they are formed like a bullet point in a list without trying form adhere to rules of sentence structures, then a period may look odd.

Let's decide for one approach and where necessary adapt messages to follow it. My guess is that a sentence structure allows us to be more versatile in how to formulate error messages. It is also something that can displayed in an alert, as it is a complete sentence.

[fn-faisal]

@mtrezza I agree. I'll update the messages to have a dot at the end.

[mtrezza]

Alright, sounds good

[mtrezza]

@fn-faisal Please let me know if this is ready for review.

[fn-faisal]

@mtrezza this PR is ready for review

[mtrezza]

Thanks, could you take a look at the failing tests?

[mtrezza]

It seems to pass! Please request a review whenever this is ready.

[fn-faisal]

@mtrezza this is ready for review

[mtrezza]

@faisal154 @fn-faisal is this ready for review? it would be great if we could get this merged and then close the related dashboard issue.

[fn-faisal]

@mtrezza yes, this PR is ready for review.

[mtrezza]

@fn-faisal It seems that some tests are still failing, could you take a look? And maybe comment on the open review questions, so we know which ones are done. I would rather merge his soon, otherwise there may be other changes in the meantime that make it more difficult to merge.

[fn-faisal]

@mtrezza sorry, there were some urgent issues at work. I'll start looking into resolving the test cases.

[mtrezza]

Great, no worries, whenever you are ready!

[fn-faisal]

@mtrezza I'm seeing notContainedIn false queries test pass for mongoDB but fail for Postgres. It fails saying the error message should contain a . at the end but the message, I didn't reformat in code as I couldn't find any reference for it, except in tests. This may be in the SDK. And the message is different for MongoDb tests and Postgres tests that's why the tests fail.

[fn-faisal]

@mtrezza I have added checks for the tests that were failing for POSTGRES. Please let me know if the PR requires any updates.

[fn-faisal]

@mtrezza I have reverted the changes and created an issue for the failing tests. https://github.com/parse-community/parse-server/issues/7732

[fn-faisal]

@mtrezza I have made the updates and previously, all tests but the codecov ones were passing. I pushed some commits and there's one postgres test that sometimes fails. https://github.com/parse-community/parse-server/blob/alpha/spec/ParseGraphQLServer.spec.js#L2603

There's a note on top of the test. Should I create an issue for this?

[mtrezza]

@fn-faisal some tests are flaky, we can just ignore them in this PR.

[fn-faisal]

Could you take a look at the previous comments, like #7446 (comment). Maybe you did not see some of the comments because they were folded in the thread. These comments mostly require renaming the error keys, to make it easier to understand what they are used for. It would then also be good if you could sort the error messages alphabetically, so we have a list that is easy to read.

Thanks for the effort, this is quite a large change, so it takes some revisions. @mtrezza I have gone through the comments and updated the PR.

[mtrezza]

@fn-faisal Apologies for the delay, I should look over this over the next days, it's quite a big (and great ;) PR.