Initial support new messages grouped by severity #736

garcia-jj · 2014-08-08T00:57:29Z

As discussed in dev mailing list many months ago.

At this time we have only errors. These errors is a List of Messages. But some applications needs to messages like warning and info. We have a Severity enum with properties like INFO and WARN, but this messages is not used by VRaptor.

With this pull request we can simple add a Message into Messages managed bean. So in the view we can use someting like this:

<c:forEach items="${messages.errors}" ... />
<c:forEach items="${messages.warn}" ... />

Categorized messages are available too: ${messages.warn.from('client.name')}, and more.

This change has backward compatibility because messages.errors is exported to view with alias errors, that is the same object.

This is an initial suggestion, and we can improve it. After, javadocs and site docs will write.

Thank you.

garcia-jj · 2014-08-08T00:58:57Z

vraptor-core/src/main/java/br/com/caelum/vraptor/validator/Messages.java

+	public List<Message> getErrors() {
+		return get(Severity.ERROR);
+	}
+


I don't know if pluralization of info and warn are right, so I wrote in singular. Suggestions?

If I'm not wrong, info can be used in singular and plural...

I'd pluralize all of them

or use errorList, warnList, infoList

I'll leave info because as @renanigt said, already in plural. But warn can be renamed to warnings.

I prefer warnings too.

renanigt · 2014-08-08T01:41:14Z

Cool ! 👍

rponte · 2014-08-08T10:44:45Z

This is a good feature!

On Thu, Aug 7, 2014 at 10:41 PM, Renan Montenegro notifications@github.com
wrote:

Cool ! [image: 👍]

—
Reply to this email directly or view it on GitHub
#736 (comment).

Rafael Ponte
TriadWorks | Formação Java
http://cursos.triadworks.com.br

lucascs · 2014-08-08T16:54:50Z

vraptor-core/src/main/java/br/com/caelum/vraptor/validator/Messages.java

+@RequestScoped
+public class Messages {
+
+	private Map<Severity, List<Message>> messages = new HashMap<>();


you could use a guava Multimap here.

I prefer using core Java libraries. But you right, because it's too uggly signature Map<X, List<Y>>. I'll change it soon. Thank you.

Hmm, I'm not able to use ListMultimap because this class creates an ArrayList, but we need to create a MessageList(ArrayList) as you can see here: https://github.com/caelum/vraptor4/pull/736/files#diff-3c30e5f5d8b7bd61d2561087bc62a05fR41

Suggestions?

garcia-jj · 2014-08-08T20:06:18Z

A method to get all messages can be useful?

public List<Message> getAll() {
    List<Message> all = new ArrayList<>();
    all.addAll(getErrors());
    all.addAll(getInfo());
    all.addAll(getWarnings());
    return all;
}

renanigt · 2014-08-08T20:20:17Z

I think can be useful, may be someone wants to get all errors without by Severity.

renanigt · 2014-08-08T21:41:55Z

Since now we have info, warnings and errors, what about to create methods, something like onWarnRedirectTo(...), onInfoRedirectTo(...) ?

So, with this we can treat each messages differently...

renanigt · 2014-08-09T00:07:14Z

Or we can also create something like:

validator.on(INFO).redirectTo(...)
validator.on(WARN).redirectTo(...)
validator.on(ERROR).redirectTo(...)

garcia-jj · 2014-08-09T18:25:48Z

Or something like validator.on(Severity.INFO, Severity.ERROR).redirectTo(...). The same for forwardTo, pageOf and badRequest. But we can improve in another pull request, to delivery this pull request as soon as possible.

Turini · 2014-08-11T15:10:22Z

@garcia-jj this feature should come in RC2?

A method to get all messages can be useful?

totally

note: this is a really nice PR! Thanks for remember about it :)

garcia-jj · 2014-08-11T18:48:17Z

@garcia-jj this feature should come in RC2?

I'm not sure. By this feature can wait if necessary.

garcia-jj · 2014-08-11T18:50:15Z

If I'm not wrong, this pull request blocks #725

renanigt · 2014-08-11T19:01:12Z

@garcia-jj, in conversation with @lucascs, he asked to wait your merge, because we need this PR to fix the bug.

So, you can merge it for we can fix #725.

renanigt · 2014-08-11T19:02:18Z

The comment is here.

garcia-jj · 2014-08-11T23:06:42Z

Regarding method List<Message> getAll() I think in message order. When a Message is added, we store in a map grouped by Severity. So we can't have information about original order. There is a problem to return errors first, info and in the last positions, warn?

Turini · 2014-08-11T23:45:45Z

There is a problem to return errors first, info and in the last positions, warn?

the order: errors, info and warns looks fair enough 👍

rponte · 2014-08-12T01:08:59Z

Hoje about using the same order of Log4j? Errors, warnings and info?

On Monday, August 11, 2014, Rodrigo Turini notifications@github.com wrote:

There is a problem to return errors first, info and in the last positions,
warn?

the order: errors, info and warns looks fair enough [image: 👍]

—
Reply to this email directly or view it on GitHub
#736 (comment).

Rafael Ponte
TriadWorks | Formação Java
http://cursos.triadworks.com.br

renanigt · 2014-08-12T01:32:58Z

I agree with @rponte. 👍

Turini · 2014-08-12T12:25:46Z

for me, the bullet point is errors being the first one :)

garcia-jj · 2014-08-12T15:23:56Z

Method added with 398c620

garcia-jj · 2014-08-12T15:30:28Z

The Messages class is just a pojo. May important to add a unit test class?

renanigt · 2014-08-12T15:39:57Z

I think isn't too important, but a plus.

garcia-jj · 2014-08-12T18:27:51Z

@Turini we merge this pull request before new release, or wait for more comments and ideas?

Turini · 2014-08-12T18:31:26Z

lets wait for more comments :) before the release we can merge

Initial support new messages grouped by severity

Turini · 2014-08-13T14:22:30Z

thanks! As discussed on mail list, it will be available at RC2 version :)

Turini · 2014-08-15T14:38:52Z

vraptor-core/src/main/java/br/com/caelum/vraptor/validator/Messages.java

+ */
+@Named
+@RequestScoped
+public class Messages {


guys, "messages" is a really generic name, it is causing some problems
at our internal projects (there is a lot of "messages" vars included to JSP)
What about change it to "VRaptorMessages" or something? It'll help a lot

Makes total sense. :+1

On Friday, August 15, 2014, Rodrigo Turini notifications@github.com wrote:

In
vraptor-core/src/main/java/br/com/caelum/vraptor/validator/Messages.java:

+/**

* Managed class that stores all application messages like errors, warnings and info. This

* class is useful to display messages categorized by severity in your view. To choose a severity

* you can construct your message like this:

*
* Message message = new SimpleMessage("name", "An info message", Severity.INFO); * validation.add(message); // will stored as INFO severity
*

* @SInCE 4.1

* @author Otávio S Garcia

*/
+@nAmed
+@RequestScoped
+public class Messages {

guys, "messages" is a really generic name, it is causing some problems
at our internal projects (there is a lot of "messages" vars included to
JSP)
What about change it to "VRaptorMessages" or something? It'll help a lot

—
Reply to this email directly or view it on GitHub
https://github.com/caelum/vraptor4/pull/736/files#r16295214.

👍

It also kind of breaks compatibility (other apps could have a named component with the same name)

errors is generic too, and we use since 3.0 version. I vote to leave as messages. Or if more people vote to rename, instead of messages we can export warnings and info, or a better name than vraptorMessages, that is too ugly and we need to use upper/lower case, because is too boring to type in JSP.

Then we should also rename warnings and info, too.

In https://github.com/caelum/mamute, for example, we had were including an attribute named "messages" in our views and we broke our app when updated vraptor :-(

I think that this will happen for other users, too.

we need to solve this question before the next release... more people think we should
keep as it is? (only messages). If not, which better name instead of vraptorMessages?

I prefer VRaptorMessages, but I was out theses days.

@renanigt we won't rename the class, but only export to view with another name to avoid conflicts.

@Turini I really dislike using upper/lower in EL. And using framework name in variable is too uggly (my point, of course). May we can use vmessages that sounds something like validation messages and vraptor messages. Or we can improve this case with idea from JSF, that allow devs to configure the name of variable exported. What you think?

vmessages looks good to me :) anyone disagree with it?

Now I see the point @garcia-jj. 😃

I agree with @Turini, vmessages looks nice. 👍

felipeweb · 2014-08-19T23:28:21Z

vmessages nice 👍

Turini · 2014-08-20T10:17:01Z

nice, thank for the opinions guys!

lucascs · 2014-08-20T17:22:55Z

vraptor-core/src/main/java/br/com/caelum/vraptor/validator/Messages.java

+ * @since 4.1
+ * @author Otávio S Garcia
+ */
+@Named


shouldn't it be changed to @Named("vmessages")?

Was made in another PR: #749

nykolaslima · 2014-08-20T22:48:22Z

awesome!!!!! 👍 👍 👍

Initial support new messages grouped by severity

0a7b32c

garcia-jj reviewed Aug 8, 2014
View reviewed changes

Extracting method to a better name

63e98f6

renanigt mentioned this pull request Aug 8, 2014

Fixing problem: validation errors ignored on redirects. Closes #476 #725

Merged

lucascs reviewed Aug 8, 2014
View reviewed changes

Adding some docs about Messages class

42a6d07

garcia-jj added 3 commits August 9, 2014 15:33

Explaining how messages works

6e2e913

Pluralizing warn to warnings

640fa5f

Adding license

b477c5a

Turini added the enhancement label Aug 10, 2014

Adding method to get all messages

398c620

garcia-jj added 2 commits August 12, 2014 14:33

Typo

6ee8b09

Typo variable

df331cf

Turini added a commit that referenced this pull request Aug 13, 2014

Merge pull request #736 from caelum/ot-messagesbyseverity

88a8434

Initial support new messages grouped by severity

Turini merged commit 88a8434 into master Aug 13, 2014

Turini deleted the ot-messagesbyseverity branch August 13, 2014 14:21

garcia-jj mentioned this pull request Aug 15, 2014

Create docs about new Messages feature #744

Closed

Turini reviewed Aug 15, 2014
View reviewed changes

lucascs reviewed Aug 20, 2014
View reviewed changes

renanigt mentioned this pull request Aug 25, 2014

Create redirect methods for Info and Warnings on validator... #759

Closed

Initial support new messages grouped by severity #736

Initial support new messages grouped by severity #736

Conversation

garcia-jj commented Aug 8, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

renanigt commented Aug 8, 2014

rponte commented Aug 8, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garcia-jj commented Aug 8, 2014

renanigt commented Aug 8, 2014

renanigt commented Aug 8, 2014

renanigt commented Aug 9, 2014

garcia-jj commented Aug 9, 2014

Turini commented Aug 11, 2014

garcia-jj commented Aug 11, 2014

garcia-jj commented Aug 11, 2014

renanigt commented Aug 11, 2014

renanigt commented Aug 11, 2014

garcia-jj commented Aug 11, 2014

Turini commented Aug 11, 2014

rponte commented Aug 12, 2014

renanigt commented Aug 12, 2014

Turini commented Aug 12, 2014

garcia-jj commented Aug 12, 2014

garcia-jj commented Aug 12, 2014

renanigt commented Aug 12, 2014

garcia-jj commented Aug 12, 2014

Turini commented Aug 12, 2014

Turini commented Aug 13, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felipeweb commented Aug 19, 2014

Turini commented Aug 20, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nykolaslima commented Aug 20, 2014