Skip to content
This repository was archived by the owner on Mar 26, 2024. It is now read-only.

(#7869) Fix missing diff in dashboard Event tab #70

Closed
wants to merge 1 commit into from
Closed

(#7869) Fix missing diff in dashboard Event tab #70

wants to merge 1 commit into from

Conversation

eshamow
Copy link
Contributor

@eshamow eshamow commented Dec 11, 2011

Modify event message to replace the word "changed"
to link to a diff against the previously stored
version. Needs some additional logic to prevent
calling against a diff that wasn't previously
stored, but this is no different from current
behavior and should be refactored separately.

Modify spec file to match this behavior.

Modify event message to replace the word "changed"
to link to a diff against the previously stored
version.  Needs some additional logic to prevent
calling against a diff that wasn't previously
stored, but this is no different from current
behavior and should be refactored separately.
@nicklewis
Copy link
Contributor

I haven't checked this yet, but it looks like this might try to link anything with the word "changed" in it to a file diff, not just file content? Perhaps an if hashes.present? check before subbing in the link. Although it may actually be better to check that there are two hashes, since the code assumes that.

@eshamow
Copy link
Contributor Author

eshamow commented Dec 11, 2011

It only makes the change inside the popup_md5s function - in other words this would have already been called to evaluate the string and add md5s.

There definitely needs to be more logic around the appearance of links - even before this the first hash will display a link whether or not the previous content was filebucketed.

I still think this is worth using because it makes the advertised behavior work, but if you feel I should circle back and work on the other issues I'll do so.

Eric Shamow
Professional Services
http://puppetlabs.com/
(c)631.871.6441

On Sunday, December 11, 2011 at 6:03 PM, Nick Lewis wrote:

I haven't checked this yet, but it looks like this might try to link anything with the word "changed" in it to a file diff, not just file content? Perhaps an if hashes.present? check before subbing in the link. Although it may actually be better to check that there are two hashes, since the code assumes that.


Reply to this email directly or view it on GitHub:
#70 (comment)

@eshamow eshamow closed this Dec 13, 2011
@eshamow
Copy link
Contributor Author

eshamow commented Dec 13, 2011

OK...having discussed further with Nick, he explained that popup_md5s gets called on all text. So this is definitely problematic.

I'm going to close the pull request for now and make the code a bit smarter, then issue a new one.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants