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

News activity log #164

Merged
merged 22 commits into from
Mar 28, 2023
Merged

News activity log #164

merged 22 commits into from
Mar 28, 2023

Conversation

llunaCreixent
Copy link
Member

@llunaCreixent llunaCreixent commented Feb 7, 2023

Closes #124

The field active is added in order to preserve entries that might be useful in the future.

Tasks:

  • Create db table
  • create skeleton for routes, validation, and controllers
  • define api endpoints (only for retrieving data, not for inserting)
  • implement
  • add API documentation
  • create tests

Deploying in prod requires coordination with db migrations

@llunaCreixent llunaCreixent marked this pull request as draft February 7, 2023 10:10
@louilinn
Copy link
Collaborator

louilinn commented Feb 9, 2023

So from the myxogastria frontend we want to use the core in the same way as for transaction and trust log.
We want to return all notifications using core.activity.getLatest and a filter of 'DISABLED', 'CONNECTIONS', 'OWNERS', 'TRANSFERS', and now 'NEWS'. This method then calls the private method getNotification calling core.utils.requestIndexedDB making graph queries.
We want getNotifiations to have the option to instead call this new API endpoint though a new core.utils.requestNews ,when filter is ActicityFilterType.NEWS.

The simplest way to deal with notifications would be if the response from this API resembles the format of the notifications from the graph when calling core.utils.requestIndexedDB with data = 'activity stream', i.e. calling core.utils.getNotificationsStatus, meaning the response from the graph query:

        query: `{
          notifications(${parameters}) {
            id
            transactionHash
            safeAddress
            type
            time
            trust {
              user
              canSendTo
              limitPercentage
            }
            transfer {
              from
              to
              amount
            }
            hubTransfer {
              from
              to
              amount
            }
            ownership {
              adds
              removes
            }
          }
        }`,

The most important RESPONSE criteria is:

The returned data/response has attribute notifications which is a list of objects

Each of these objects have the following attributes:

  • type which is a string set to `'NEWS'
  • news which is an object containing
    • iconId number
    • message object with:
      • en string
      • later other attributes for translations such as fr
  • time in the same format as timestamps from the graph
  • other a tributes if necessary

Further more we don't want to include inactive news in the response.

Of course there can also be a parsing function in the core to ensure this format. So these are preferences. Let me know what you think.

REQUEST criteria

The query we want to be able to make is guided by the same principles as for the graph notification queries, which has the following parameters:

orderBy: "time",
orderDirection: "desc",
first: ${options.limit},
skip: ${options.offset},
where: {
    time_gt: ${options.timestamp},
    safeAddress: "${options.safeAddress.toLowerCase()}",
    ${filterString ? `type: ${filterString}` : ''}
}
  • we want to always order by time starting with the latest
  • we want to include some options in our request to the endpoint:
    • limit - pagination page size (default 10)
    • offset - pagination start index (default 0)
    • timestamp - show only messages after this time (less important, unsure if we will need it) (default 0, number format preferably the same as the graph accepts)

@llunaCreixent
Copy link
Member Author

@louilinn thanks for the definition.
Then, in the api, we just have to allow the following filters:

  • active (default true)
  • afterTime (default not applied)

The rest of the formatting will be done in the core, adding:

  • type which is a string set to `'NEWS'
  • news which is an object containing each news object

For enhancement, we could add the language in the query. Right now the code assumes only en language.

@llunaCreixent llunaCreixent marked this pull request as ready for review February 14, 2023 18:56
Copy link
Collaborator

@louilinn louilinn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks very good 🌟
Requests:

  • fix the word mistake in test description
  • Please complement the test to include inactive items, see comment.
  • improve parameter docs

API.md Show resolved Hide resolved
src/controllers/news.js Show resolved Hide resolved
src/controllers/news.js Show resolved Hide resolved
test/news-find.test.js Outdated Show resolved Hide resolved
@llunaCreixent llunaCreixent requested a review from louilinn March 27, 2023 13:46
@llunaCreixent llunaCreixent merged commit 3841143 into main Mar 28, 2023
@llunaCreixent llunaCreixent deleted the news-activity-log branch March 28, 2023 16:03
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.

Frontend feature: introduce database and endpoint for app updates in activity log
2 participants