Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug 515 fix site url must be absolute #125

Conversation

byrnereese
Copy link
Member

No description provided.

@jayallen
Copy link
Member

Hey Byrne, can you rebase this against the most recent openmelody/master.

git remote update
git co bug-515-fix-site-url-must-be-absolute
git rebase openmelody/master

Then, all of those checkins, if they remain separate (which is fine) need Lighthouse declarations so:

git rebase -i openmelody/master    # + reword

Please see the Contributor Guidelines section on Commit_Documentation for details of what I'm looking for.

…template screen. This is effectively a no-op with Theme Manager installed, so you will see another commit in Theme Manager that replicates this. This must be made in case anyone ever removes Theme Manager.
…of supported methods and adding the backend logic as well. There was a TODO item to break this out into a separate handler, and that is what I have done.
…be an absolute URL."

This is the result of some outdated logic whereby in order to change the publishing profile of a blog, one would invoke the MT::CMS::Blog::save method/handler. There was a long standing to do to move this to a dedicated handler, which is what we have done here. This bypasses the cms_pre_blog_save_filter or somesuch that was triggering the error. It was safe to bypass because we were not saving the entire blog - we were just updating its profile.
…r. This was done in order to resolve a merge conflict with Theme Manager's config.yaml file.
@byrnereese
Copy link
Member Author

Done.

@jayallen
Copy link
Member

Did you repush -f the branch after the rewording because I'm still seeing commits without Lighthouse declarations.

@byrnereese
Copy link
Member Author

I think I mis-understood - must every commit bear the [#515] text to link the commit to the bug? I was just adding the [#515 state:resolved] to the last commit.

@jayallen
Copy link
Member

Yep. Think of it this way: Only commits with Lighthouse declarations get attached to the lighthouse ticket. Also, if any old person goes to say, this commit on GitHub, they have no way of knowing the full context of that change without seeing the lighthouse ticket #. Further, the person who compiles the release notes (hi! :-) has that same problem.

I know I wrote this somewhere in even more detail than on the page I linked to above but I can't figure out where. Essentially branches with multiple commits for the same ticket should look like this:

  • #NNN state:open
  • #NNN
  • #NNN
  • #NNN (and so on...)
  • #NNN state:resolved

Multiple commits done in this way are perfectly fine even if they are small. However, if it makes sense to squash them together, and detail all of the changes in a single commit note, you're welcome to do that too. The only caveat is not to let any one commit get TOO big or do too many things at once because having separate commits makes it very easy to revert just a single commit if it's found to be necessary later on.

@jayallen
Copy link
Member

I went ahead and rebased this branch, rewrote all the commits and pushed. You can delete it now.

This pull request was closed.
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.

2 participants