Skip to content

MAS-129 Daily email all fields no branding#34

Open
d-lan1 wants to merge 8 commits intomasterfrom
MAS-129-DailyEmailFilds
Open

MAS-129 Daily email all fields no branding#34
d-lan1 wants to merge 8 commits intomasterfrom
MAS-129-DailyEmailFilds

Conversation

@d-lan1
Copy link

@d-lan1 d-lan1 commented Jan 2, 2020

@d-lan1 d-lan1 force-pushed the MAS-129-DailyEmailFilds branch from 9cb6d58 to 94e9a23 Compare January 3, 2020 13:33
@d-lan1 d-lan1 force-pushed the MAS-129-DailyEmailFilds branch from 94e9a23 to 6adbe19 Compare January 3, 2020 13:56
}
};

var expectedHtml = "<div class='evidenceType'><strong>Some evidence type</strong><div class='item'>Some Title<br>Some source<br>Some speciality<br>Some short summary<br><a href='https://www.medicinesresources.nhs.uk/abc'>SPS Comment</a></div></div>";
Copy link
Contributor

Choose a reason for hiding this comment

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

Should use the extension method ShouldMatchApproved

Copy link
Author

@d-lan1 d-lan1 Jan 6, 2020

Choose a reason for hiding this comment

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

good shout!

@@ -67,7 +67,7 @@ private string CreateContentBody(Item item)
contentBody.Append(Environment.NewLine);

contentBody.Append("UKMI Comment: ");
Copy link
Contributor

Choose a reason for hiding this comment

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

need to remove UKMI

}


public string HTML
Copy link
Contributor

@suegarner suegarner Jan 6, 2020

Choose a reason for hiding this comment

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

Not sure why this is a model

Copy link
Author

Choose a reason for hiding this comment

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

It makes sense to me to have a daily email model. I agree with you that HTML property may not belong in this class - but im unsure as I've seen business logic in models before and the HTML is after all just a string property returning a string interpretation of the rest of the class. The HTML properties logic could be moved into a separate service which perhaps returns a dynamic view. However, for this card I think what we currently have is ok. Perhaps in the branding card, we can look at this. What do you think?

Copy link
Author

@d-lan1 d-lan1 left a comment

Choose a reason for hiding this comment

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

please see comments. - Weird how my comments have resulted in a review :S not sure what happened there.

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