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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃悰Fix access to the common article for multiple editors #10214

Merged
merged 2 commits into from
Dec 3, 2018
Merged

馃悰Fix access to the common article for multiple editors #10214

merged 2 commits into from
Dec 3, 2018

Conversation

kacperduras
Copy link
Contributor

Fixes #10212
/cc @gargol

@naz
Copy link
Member

naz commented Nov 22, 2018

That was quick @kacperduras, thanks! I'll try to look into this next week 馃槂 Couple thing that would suggest changing/adding right away:

  • following our commit message naming convention
  • and adding few unit tests for the bug this change is fixing (you'd need to create a new suite as there have been no unit tests for this module)

Feel free to reach out if you have any questions.

@kacperduras
Copy link
Contributor Author

Ok, thanks a lot. I haven't got a time in this moment, but tomorrow - no problem :D

@kacperduras kacperduras changed the title GH-10212: Fix access to the common article for multiple editors 馃悰Fix access to the common article for multiple editors Nov 23, 2018
@kacperduras
Copy link
Contributor Author

kacperduras commented Nov 23, 2018

@gargol

[...] following our commit message naming convention

Fixed.

[...] and adding few unit tests for the bug this change is fixing (you'd need to create a new suite as there have been no unit tests for this module)

Any suggestion for the location of the file?

@rishabhgrg
Copy link
Contributor

rishabhgrg commented Nov 26, 2018

Hey @kacperduras 馃憢

Small fix needed for the commit messages, they need to be in past tense as per the convention, so Fixed in this case. You can also combine these 2 commits into a single commit.

For the location of test file, I'd suggest core/test/unit/models/relations/authors_spec.js as the rest of test folder structure mimics the structure of main file. 馃槃

@naz
Copy link
Member

naz commented Dec 3, 2018

@kacperduras any progress on the unit tests? 馃槂

@kirrg001
Copy link
Contributor

kirrg001 commented Dec 3, 2018

@gargol Hey 馃憢How about

  • you double check the requested code changes again
  • merge if it looks good to you
  • and if a unit test is 100% required and helps avoiding critical bugs, push one test straight to master afterwards (or to this branch) :)

Would love to see this commit in master and in the release tomorrow 馃憤 馃ぉ馃懟

@naz naz merged commit 7c1840f into TryGhost:master Dec 3, 2018
allouis added a commit that referenced this pull request Dec 4, 2018
* master:
  Version bump to 2.7.0
  Updated Ghost-Admin to 2.7.0
  馃悰 Fixed contributors being able to delete draft posts as co-author (#10239)
  Added configuration controller to v2 API (#10056)
  馃悰 Fixed site changed webhook not triggered for scheduled posts
  馃悰 Fixed invalid imported subscribers count (#10229)
  馃悰Fixed auto redirect for extra data queries on post and page resources (#9828)
  Included relations if static resource is post | page (#10148)
  Renamed API -> Api for v2 auth logic (#10142)
  馃悰Removed user reference warning from importer report if post is a draft (#10169)
  馃悰 Fixed edit permission of the common article by multiple authors (#10214)
  Updated npm keywords (#10217)
@kacperduras
Copy link
Contributor Author

@gargol Sorry, but I was very busy and I forgot :\ I promise that it will not happen again :)

@kacperduras kacperduras deleted the issue-10212 branch December 4, 2018 19:07
allouis added a commit that referenced this pull request Dec 7, 2018
* master:
  Version bump to 2.7.1
  Updated Ghost-Admin to 2.7.1
  Version bump to 2.7.0
  Updated Ghost-Admin to 2.7.0
  馃悰 Fixed contributors being able to delete draft posts as co-author (#10239)
  Added configuration controller to v2 API (#10056)
  馃悰 Fixed site changed webhook not triggered for scheduled posts
  馃悰 Fixed invalid imported subscribers count (#10229)
  馃悰Fixed auto redirect for extra data queries on post and page resources (#9828)
  Included relations if static resource is post | page (#10148)
  Renamed API -> Api for v2 auth logic (#10142)
  馃悰Removed user reference warning from importer report if post is a draft (#10169)
  馃悰 Fixed edit permission of the common article by multiple authors (#10214)
  Updated npm keywords (#10217)
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.

None yet

4 participants