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

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

Open
wants to merge 87 commits into
base: alpha
Choose a base branch
from

Conversation

fn-faisal
Copy link
Member

New Pull Request Checklist

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.
parse-community/parse-dashboard#1731 (comment)

@mtrezza
Copy link
Member

mtrezza commented Jun 25, 2021

Not sure about this PR, see parse-community/parse-dashboard#1731 (comment).

@fn-faisal
Copy link
Member Author

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.

@mtrezza mtrezza added type:bug Impaired feature or lacking behavior that is likely assumed and removed type:bug Impaired feature or lacking behavior that is likely assumed labels Jul 11, 2021
@fn-faisal
Copy link
Member Author

@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
Copy link
Member

mtrezza commented Aug 31, 2021

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
Copy link

codecov bot commented Sep 1, 2021

Codecov Report

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

Impacted file tree graph

@@            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%) ⬆️
src/Adapters/Storage/Mongo/MongoTransform.js 88.12% <62.50%> (+0.01%) ⬆️
src/RestWrite.js 94.20% <71.42%> (ø)
src/Routers/UsersRouter.js 94.41% <82.35%> (+0.03%) ⬆️
src/Adapters/Storage/Mongo/MongoStorageAdapter.js 93.37% <83.33%> (+0.01%) ⬆️
src/Errors/message.js 84.84% <84.84%> (ø)
...dapters/Storage/Postgres/PostgresStorageAdapter.js 95.46% <92.85%> (+<0.01%) ⬆️
src/AccountLockout.js 97.61% <100.00%> (+0.05%) ⬆️
src/RestQuery.js 95.71% <100.00%> (+<0.01%) ⬆️

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
Copy link
Member

mtrezza commented Sep 10, 2021

@fn-faisal Is this ready for review?

@fn-faisal
Copy link
Member Author

@mtrezza Yes, it's ready for review.

@mtrezza mtrezza changed the title Message error for required fields #1731 refactor: error message reformatted and moved into own files (#1731) Sep 20, 2021
@parse-github-assistant
Copy link

parse-github-assistant bot commented Sep 20, 2021

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 mtrezza changed the title refactor: error message reformatted and moved into own files (#1731) refactor: error messages reformatted and moved into own files (#1731) Sep 20, 2021
@mtrezza
Copy link
Member

mtrezza commented Sep 20, 2021

  • 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?

src/Errors/message.js Outdated Show resolved Hide resolved
src/Errors/message.js Outdated Show resolved Hide resolved
src/Errors/message.js Outdated Show resolved Hide resolved
src/Errors/message.js Outdated Show resolved Hide resolved
src/Errors/message.js Outdated Show resolved Hide resolved
Copy link
Member

@mtrezza mtrezza left a comment

Choose a reason for hiding this comment

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

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 mtrezza mentioned this pull request Dec 6, 2021
2 tasks
@fn-faisal
Copy link
Member Author

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.

@fn-faisal fn-faisal requested a review from mtrezza December 14, 2021 13:21
@mtrezza
Copy link
Member

mtrezza commented Jan 8, 2022

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

@fn-faisal
Copy link
Member Author

@mtrezza NP 👍 . I've gone through the requested changes have updated the PR as requested. Please let me know if any other updates are required.

@fn-faisal
Copy link
Member Author

@mtrezza I have resolved some of the merge conflicts for the PR. Please let me know of any required updates for the PR once you review it.
Thanks.

@mtrezza
Copy link
Member

mtrezza commented Jan 20, 2022

Thanks @fn-faisal, I'll review in the coming days, apologies for the delay.

@fn-faisal
Copy link
Member Author

@mtrezza ok, sounds good. thanks.

@dblythy
Copy link
Member

dblythy commented Jan 8, 2023

What is pending here @mtrezza?

@mtrezza
Copy link
Member

mtrezza commented Jan 8, 2023

It needs a rebase and a complete review of the suggested changes.

@fn-faisal
Copy link
Member Author

Hi guys,
I made this PR about a year ago back when I was working at Back4App but as I'm no longer working with them and have moved on to better opportunities, I'm afraid I won't be able to invest time on the open issues anymore.
@mtrezza if you'd like, I'm ok with closing this PR or if someone else would like to pick this one up, that's cool too.

@mtrezza
Copy link
Member

mtrezza commented Jan 8, 2023

Thanks for the heads up, I guess we can keep it open for someone else to continue it.

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