Chongsun Ahn

I am curious about the decision for some log statements to be errors? Some of them seem like warnings to me. Like in NotificationTransformer or NotificationChannelRouter.

I am curious about the decision for some log statements to be errors? Some of them seem like warnings to me. Like in NotificationTransformer or NotificationChannelRouter.

We may have SMS support in the future, but we can still test that nothing is returned if an SMS channel is specified and there is no SMS message in the notification.

We may have SMS support in the future, but we can still test that nothing is returned if an SMS channel is specified and there is no SMS message in the notification.

I think you mean cannot extract notification message, as this component does not send a notification.

I think you mean cannot extract notification message, as this component does not send a notification.

I meant setting a variable in the given section of each test, as opposed to creating constants.

I meant setting a variable in the given section of each test, as opposed to creating constants.

This test seems odd to me. *The test name says it should return an empty set, but the assertion is for null. *It seems odd to test using a null channel header. If we want to test that a notificat...

This test seems odd to me.

  • The test name says it should return an empty set, but the assertion is for null.
  • It seems odd to test using a null channel header. If we want to test that a notification message is not found for a channel, perhaps we would set up the integration message to have an empty payload, but specify the matching channel. Or have a payload, but with a different notification channel (SMS). Perhaps there are multiple test cases here for nothing to extract.
  • Question: if the notification had a channel of null and this channel to use header is set to null, would it return null, or would the channels match and it return the integration message?
We should name this extractNotificationMessage, since we are just extracting a single message.

We should name this extractNotificationMessage, since we are just extracting a single message.

I would prefer that we define an important variable, set it to true or false and pass it in. Just looking at this code, it is unclear what we are passing in as true or false. I had to look at the f...

I would prefer that we define an important variable, set it to true or false and pass it in. Just looking at this code, it is unclear what we are passing in as true or false. I had to look at the filter code to determine that. Same with the tests below.

Do we know this will not retrieve the same pending notification over and over if the notification does not get deleted/cleared for any reason?

Do we know this will not retrieve the same pending notification over and over if the notification does not get deleted/cleared for any reason?

What if the user's contact details is not found? Wouldn't this create a NPE?

What if the user's contact details is not found? Wouldn't this create a NPE?

Have we considered a case where a pending notification is added, but the user is configured not to allow notifications? Doesn't that mean the notification stays pending indefinitely? And if the use...

Have we considered a case where a pending notification is added, but the user is configured not to allow notifications? Doesn't that mean the notification stays pending indefinitely? And if the user ever turns on allowing notifications in the future, the user would get all the pending notifications that have built up? Is there a situation where we would want to process a pending notification and just not send it if the user has turned it off?

I see that the design has changed from a send flag to a new model called pending notification. Is there documentation for the design change? Things that were considered? Pros and cons?

I see that the design has changed from a send flag to a new model called pending notification. Is there documentation for the design change? Things that were considered? Pros and cons?

We should change all the System.out.println statements to log statements.

We should change all the System.out.println statements to log statements.

Does this method have to be named test? If not, we should make it more descriptive.

Does this method have to be named test? If not, we should make it more descriptive.

Seems like "sent" would be more clear, to indicate that a message had been sent.

Seems like "sent" would be more clear, to indicate that a message had been sent.

I am not sure what would be a better naming scheme. The default request details seems like it is more about permission string based request details and the requisition one includes comparing nodes ...

I am not sure what would be a better naming scheme. The default request details seems like it is more about permission string based request details and the requisition one includes comparing nodes and also if a requisition is a partner requisition.

It feels like this permission checking logic is too complex and that we should be creating subclasses for different ways to check permissions; one would be permission string based only, and the other would do extra checks. Each would implement some hasPermission() method and return true if the user has permission. The details of what is checked could be abstracted. However, I understand that seems like a lot of work to refactor.

At the very least, if we can't think of better names, I would prefer we add some docs about what each of these classes are for. I leave it up to you how you want to handle this issue.

I understand that we need this for checking approval and viewing. However it seems like we would only need to use the facility/program combo for initiate, submit and authorize. It would be nice to ...

I understand that we need this for checking approval and viewing. However it seems like we would only need to use the facility/program combo for initiate, submit and authorize. It would be nice to clearly indicate that in the naming of this method. However, I will leave it up to you.

Actually I think we do want to check for matching supervisory supervision role for viewing, so this seems to be the correct method call.

Actually I think we do want to check for matching supervisory supervision role for viewing, so this seems to be the correct method call.

See response above.

See response above.

See response above.

See response above.

I am wondering why we don't use the the facility/program version of checkSupervisionPermission?

I am wondering why we don't use the the facility/program version of checkSupervisionPermission?

This interface needs to be more descriptive; what request details? What kind of request are we talking about here? Same thing with the implementing classes; what are default request details? What a...

This interface needs to be more descriptive; what request details? What kind of request are we talking about here? Same thing with the implementing classes; what are default request details? What are requisition request details? We know currently, but someone new looking at the code will not be able to figure it out based on the names.

Are we sure this is correct? Doesn't this check also check for a matching supervisory supervision role?

Are we sure this is correct? Doesn't this check also check for a matching supervisory supervision role?

Are we sure this is correct? Doesn't this check also check for a matching supervisory supervision role? Same with the other checks in this method.

Are we sure this is correct? Doesn't this check also check for a matching supervisory supervision role? Same with the other checks in this method.

Are we sure this is correct? Doesn't this check also check for a matching supervisory supervision role?

Are we sure this is correct? Doesn't this check also check for a matching supervisory supervision role?

Are we sure this is correct? Doesn't this check also check for a matching supervisory supervision role?

Are we sure this is correct? Doesn't this check also check for a matching supervisory supervision role?

So I understand what the refactor is trying to do here; however someone who is not familiar with this part of the code will be easily confused. In fact, even we might be confused reading this much ...

So I understand what the refactor is trying to do here; however someone who is not familiar with this part of the code will be easily confused. In fact, even we might be confused reading this much later. So I get check supervision, fulfillment and general permission methods, since their parameters follow those types of permission strings. However, this one is confusing. It may not be immediately clear to someone what a requisition has to do with checking supervision permissions. And which checkSupervisionPermission function should be used in which case? We should rename this one to indicate it is for checking requisitions during the approval process. Something like checkPermissionForRequisitionApproval.