hdgarrood / redmine_release_notes

A plugin to add release notes to Redmine
GNU General Public License v3.0
73 stars 47 forks source link

Issue #70 -bring back cf-updating check box. #77

Closed dipanm closed 11 years ago

dipanm commented 11 years ago

...ch automatically updates the issue custom field.

Mostly solves issue #70. The only thing not working is the redirect to issue page on update.

hdgarrood commented 11 years ago

FYI -- I've done a little bit of work on top of these commits, and pushed it to the branch pull-77. If you're doing any more work on this, you might want to base it off that.

The issue of what to do after submitting a change to release notes (whether successfully or not) is more difficult than I first thought -- if it succeeds, we need to re-render the custom field and the issue history. Doing that with JS seems unnecessarily complex so I'd rather refresh the whole page.

However, if something goes wrong, (eg attempt to enter blank release notes, plugin misconfiguration, issue was deleted, edit issues permission was revoked, custom value failed to save), we need to be able to display the errors, too. Since the release notes form is hidden by default, and assuming we go for the no ajax route, we'd need some extra plumbing to get it to display, with the errors, if there are any.

It might work to put some javascript in the update.js.erb response, which refreshes the page only if everything is ok.

dipanm commented 11 years ago

I was unable to manage how to bring the pull-77 branch work on my master! So I have applied it by manually applying the patch and commit back. I will finish the balance work now.

hdgarrood commented 11 years ago

haha, fair enough. I think you can do something like:

git remote add git://github.com/hdgarrood/redmine_release_notes.git hdgarrood
git fetch hdgarrood
# at this point, ensure you're on the branch you want to merge the changes into
git merge hdgarrood/pull-77
dipanm commented 11 years ago

Thanks - i wasted a lot of time so went brute force! (every thing is unfamiliar to me here :( - the RoR, the Git, the Github ... but its' fun doing and learning.

OK - have added a function in issue patch as we discussed earlier- so most of your comments are taken care of. What finally remains to be done is the re-rendering the whole page when CF changes. I have no clue how to do it.

One more thing - I wanted to ask - while we are doing update_custom_field() inside the control code. However, won't it to be better that this is a method inside the issue patch - something like issue.mark_release_note_complete(ture/false) what do you think?

dipanm commented 11 years ago

What is the next step now? can we merge this pull-request - or you expect me to put up something more? Once this closes #70, I want to quickly move to #75 - which is very essential for me. please guide.

hdgarrood commented 11 years ago

Before I pull these changes, I want:

However if the code in your fork works for you, there's nothing stopping you using it as it is and moving to #75.

dipanm commented 11 years ago

Is this good now?

hdgarrood commented 11 years ago

awesome, thanks :) I'm going to do a little refactoring and testing myself, and then I reckon this is good to merge :)

Incidentally, by "add some tests", I meant automated ones -- Ruby code inside test/ which can verify whether it's working as expected. However testing Rails apps -- and especially Redmine plugins -- can be a real PITA... I'm going to try to add some myself.

hdgarrood commented 11 years ago

Did you manage to get either of the custom value or the journal entry to fail to save?

dipanm commented 11 years ago

Regarding the 'failed to save custom value" and "failed to journal entry" - a confession - i didn't face these errors.

In order to generate the condition of 'failed to save custom value' (but custom field does exists with issue) - i tried many things: a. the issue edit permission was changed (in the background) - to make it fully non-editable by the given role. Surprisingly, the CF would still save. It wasn't wrong because custom_value.save wasn't failing even though issue.save would have failed. b. I tried to bring the issue to a status where the CF field is 'Read only' - hence, i thought it would failed to save - unfortunately - even then custom_value.save didn't fail at all. Inherently, i think Redmine is not preventing custom_value.save - and if you see the model there are no validations as such.
The only thing i think was that if the field is 'required' - but if we keep it blank - it would throw an error. However, this case will never exists in our case. c. last it tried to change DB password itself. This of course fired an error - but right at a very high level. In this case, the Release note form doesn't go away and remains stuck there. Unfortunately there is no error being displayed here - but i think this situation is rather pathological anyway!

Anyway, thanks for accepting the patch. I will now work towards #75.

hdgarrood commented 11 years ago

Cool :) yeah, that's the same conclusion I came to. If journal.save or custom_value.save isn't working, it probably indicates a much bigger problem.