apostrophecms / apostrophe

A full-featured, open-source content management framework built with Node.js that empowers organizations by combining in-context editing and headless architecture in a full-stack JS environment.
https://apostrophecms.com
MIT License
4.34k stars 589 forks source link

[Apostrophe versions] authorId and author fields is null for aposDocVersions #2243

Open okfmohamed opened 4 years ago

okfmohamed commented 4 years ago

To Reproduce

Step by step instructions to reproduce the behavior:

  1. Download and install the Apostrophe Open Museum project from: https://github.com/apostrophecms/apostrophe-open-museum

  2. Add various content types from the back end modal using the open museum project.

  3. Edit any content type and click on More/Versions:

  4. Few versions are missing the user title and instead of displaying the message: admin made changes on DD/MM/YYYY it display as: made changes on DD/MM/YYYY

Expected behavior

The user title should always be displayed for each content type in the versions modal as only logged in users should be allowed to edit and make changes to the back end content using the modal

Describe the bug

The authorId and author fields are not always saved as the current logged in user and in many instances they are instead saved as null in the aposDocVersions collection for many documents:

image

Details

Version of Node.js: v12.14.0

Server Operating System: Linux (Ubuntu 18x) while no Docker is installed

Additional context:

The same issue can be replicated across different browsers and different devices (OS)

Screenshots

image

Mongo DB Dump

This DB dump shows all aposDocVersions documents where the authorIdand author fields is null for out of the box Git Hub Apostrophe boiler plate installation and no extra configuration. So far there are about 20 reported entries while in some other Prod sites the number of authorIdand author fields null records can reach +200k entries.

Also this dump has more recent entries than the one showed in the screen shot.

Attached:

mongodb.zip

boutell commented 4 years ago

It's good that you are giving examples based on open museum, but to save time if we don't happen to choose the same path, would you please be more specific about what we should do in order to reproduce it for sure? Thank you.

On Mon, Jul 13, 2020 at 4:34 PM Omar notifications@github.com wrote:

To Reproduce

Step by step instructions to reproduce the behavior:

  1. Download and install the Apostrophe Open Museum project from: https://github.com/apostrophecms/apostrophe-open-museum

  2. Add various content types from the back end modal using the open museum project.

  3. Edit any content type and click on More/Versions:

  4. Many versions are missing the user title and it instead says: made changes on DD/MM/YYYY while others correctly says: admin made changes on DD/MM/YYYY

Expected behavior

The user title should always be displayed for each content type in the versions modal as only logged in users should be allowed to edit and make changes to the back end content using the modal Describe the bug

The authorId and author fields are not always saved as the current logged in user and in many instances they are instead saved as null in the aposDocVersions collection for many documents:

[image: image] https://user-images.githubusercontent.com/39920614/87348625-10868500-c523-11ea-84a3-fe32412c3815.png Details

Version of Node.js: v12.14.0

Server Operating System: Linux (Ubuntu 18x) while no Docker was isntalled

Additional context:

The same issue can be replicated across different browsers and different devices (OS)

Screenshots

[image: image] https://user-images.githubusercontent.com/39920614/87348394-a53cb300-c522-11ea-88f9-9f75a6cf3dc1.png

Mongo DB Dump

This DB dump shows all aposDocVersions documents where the authorId and author fields is null for out of the box Git Hub Apostrophe boiler plate installation and no extra configuration. So far there are about 20 reported entries while in some other Prod sites the number of authorId and author fields null records can reach +200k entries

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/apostrophecms/apostrophe/issues/2243, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAH27KCPVMDN5HPS2ZIDB3R3NVTXANCNFSM4OY24CTA .

--

THOMAS BOUTELL | CHIEF TECHNOLOGY OFFICER APOSTROPHECMS | apostrophecms.com | he/him/his

okfmohamed commented 4 years ago

@boutell

Thank you so much for your reply

I will try to replicate the issue as I still don't fully understand how it happens but for now I can see from the date stamps in the DB dump that some of these entries are more recent than the one on the screen shot

okfmohamed commented 4 years ago

@boutell

Can this be caused by:

const req = apos.tasks.getAnonReq() OR

const req = apos.tasks.getReq()

okfmohamed commented 4 years ago

@boutell

Again I only used Apostrophe git Hub project with no custom configuration

okfmohamed commented 4 years ago

Do you think it is better to add place holder for the req object if the author field wasn't an actual user req object instead of showing it as empty?

boutell commented 4 years ago

I would think it is possible that it could mean the change was made by a task, yes.

On Mon, Jul 13, 2020 at 7:12 PM Omar notifications@github.com wrote:

Do you think it is better to add place holder for the req object if the author field wasn't an actual user req object instead of showing it as empty?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/apostrophecms/apostrophe/issues/2243#issuecomment-657859955, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAH27OEV4G4PH266P7T7T3R3OIFRANCNFSM4OY24CTA .

--

THOMAS BOUTELL | CHIEF TECHNOLOGY OFFICER APOSTROPHECMS | apostrophecms.com | he/him/his

okfmohamed commented 4 years ago

Okay thanks for the update though don't you think that as version log it should show all users making any changes to the back end content? These users don't have to be always logged in users but they can be 3rd party APIs or command line tasks

I think it would be much much better user experience that if no actual user is present in the context it would default to the name of the task that created that content so per say it could be (command line task, API request...etc) and that could be overridden on project level code to include any custom name and that would only be the case as well if there was no actual user object

Also back end users will never know why the user title was missing besides this would be very good thing to narrow down the scope of the issue as I can't tell now for sure if this is a bug or it is just code improvement/tweak?

boutell commented 4 years ago

Changing all the calls to getReq would be a big task, but changing getReq to include a title for its simulated user object would make a great PR. getAnonReq could return a different title.

On Tue, Jul 14, 2020 at 12:39 PM Omar notifications@github.com wrote:

Okay thanks for the update though don't you think that as version log it should show all users making any changes to the back end content? These users don't have to be always logged in users but they can be 3rd party APIs or command line tasks

I think it would be much much better user experience that if no actual user is present in the context it would default to the name of the task that created that content so per say it could be (command line task, API request...etc) and that could be overridden on project level code to include any custom name and that would only be the case as well if there was no actual user object

Also back end users will never know why the user title was missing besides this would be very good thing to narrow down the scope of the issue as I can't tell now for sure if this is a bug or it is just code improvement/tweak?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/apostrophecms/apostrophe/issues/2243#issuecomment-658286166, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAH27IRFDJ23R6GLUHXF6TR3SC4ZANCNFSM4OY24CTA .

--

THOMAS BOUTELL | CHIEF TECHNOLOGY OFFICER APOSTROPHECMS | apostrophecms.com | he/him/his

boutell commented 4 years ago

(I have seen developers sometimes use getReq where they really should be using the original req. Doing so will of course lead to loss of information about who did what. getReq should be reserved for times when the system is doing something on its own.)