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

Added feature to share individual input validation errors in session #553

Merged
merged 11 commits into from
Apr 28, 2022

Conversation

yubarajshrestha
Copy link
Contributor

@yubarajshrestha yubarajshrestha commented Mar 5, 2022

Fixes #552.

@girardinsamuel girardinsamuel changed the title Added feature to display individual input validation errors, fixes #552 Added feature to display individual input validation errors Mar 7, 2022
@girardinsamuel girardinsamuel changed the title Added feature to display individual input validation errors Added feature to share individual input validation errors in session Mar 7, 2022
Copy link
Contributor

@girardinsamuel girardinsamuel left a comment

Choose a reason for hiding this comment

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

IMO, this would be better to do it in its own middleware to allow a user to disable sharing validation errors.

Also errors are already shared in session through errors key. I would directly use this key instead of using validation_errors key.

So maybe, we should mix both of those approach in ShareErrorsInSession middleware:

  • Put sharing errors logic in this middleware
  • Map errors with MessageBag as you did.

We would end up with something like this:

class ShareErrorsInSessionMiddleware(Middleware):
    def before(self, request, response):
         request.app.make("view").share(
                {
                    "errors": MessageBag(Session.pull("errors") or {}),
                }
            )
        return request

    def after(self, request, _):
        return request

Copy link
Contributor Author

@yubarajshrestha yubarajshrestha left a comment

Choose a reason for hiding this comment

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

Created separate middleware to handle errors in frontend...

src/masonite/views/view.py Outdated Show resolved Hide resolved
Copy link
Contributor Author

@yubarajshrestha yubarajshrestha left a comment

Choose a reason for hiding this comment

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

Fixed requested changes.

@girardinsamuel
Copy link
Contributor

I took a look again here and I would like to add a unit test.
So I added a unit test and I got an error..I need to look a bit into it, I will try to solve this soon !

@josephmancuso
Copy link
Member

whats going on with this PR? is it good?

@girardinsamuel
Copy link
Contributor

Hmm for now there is something with the unit test I wrote, I don't know if the issue is coming from tests or from the framework itself. I will push this so we can discuss it.

@yubarajshrestha
Copy link
Contributor Author

@josephmancuso @girardinsamuel what's the status for this PR? Any issues? It's working perfectly in my case in all of my applications.

@girardinsamuel
Copy link
Contributor

what's the status for this PR? Any issues? It's working perfectly in my case in all of my applications.

I was waiting for the last release to be made, to hopefully fix the unit tests I pushed to this PR.
And I will need to review it again.

@josephmancuso
Copy link
Member

josephmancuso commented Apr 24, 2022

Put this forked branch up to date with 4.0

@josephmancuso
Copy link
Member

@josephmancuso @girardinsamuel what's the status for this PR? Any issues? It's working perfectly in my case in all of my applications.

@yubarajshrestha what's preventing me from merging it really is that failing test.

@girardinsamuel
Copy link
Contributor

It's a test I have added. I will remove it for now

@girardinsamuel girardinsamuel force-pushed the feature/552 branch 2 times, most recently from 17eddc6 to 8a875c3 Compare April 25, 2022 08:22
Copy link
Contributor

@girardinsamuel girardinsamuel left a comment

Choose a reason for hiding this comment

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

That's okay for me for now.
In Masonite 5, maybe we will need to refactor session flashing and message bag validation features.

@girardinsamuel girardinsamuel added the Next Minor Non-breaking change that can go into the next minor version label Apr 25, 2022
@josephmancuso josephmancuso merged commit 3ce3c5b into MasoniteFramework:4.0 Apr 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Next Minor Non-breaking change that can go into the next minor version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Validation Error Message for individual input fields.
3 participants