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

Gh 1659 portlet pref rest controller #1660

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

j-crawford
Copy link
Contributor

@j-crawford j-crawford commented Apr 5, 2019

Checklist
Description of change

added the Portlet Preference REST Controller to allow web-components and soffits to store and retrieve preferences

IPortletEntity entity =
portletEntityRegistry.getOrCreateDefaultPortletEntity(
request, portletDefinition.getPortletDefinitionId());
StringBuilder stringBuilder = new StringBuilder();
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel that the JSON response should be built using POJOs and converted to JSON using something such as Jackson. Is there a reason behind building the response by hand?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

@jgribonvald jgribonvald left a comment

Choose a reason for hiding this comment

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

Here a quick first review 😉
thanks for this nice purpose !

IPortletEntity entity =
portletEntityRegistry.getOrCreateDefaultPortletEntity(
request, portletDefinition.getPortletDefinitionId());
StringBuilder stringBuilder = new StringBuilder();
Copy link
Contributor

Choose a reason for hiding this comment

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

.contentType(MediaType.APPLICATION_JSON)
.body(mapper.writeValueAsString(eprefs));
} catch (IllegalArgumentException e) {
if (e.getMessage().contains("No portlet definition found for id '")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

There isn't a better error ? because intercepting only on a message is never good. As example if there is a change on the text message this interception won't work anymore and nothing can permit to track it/keep the link to the case to intercept.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There might be, but the error I'm catching is coming from the depths of uPortal itself, I didn't make that part. I'm not sure where it's coming from so I don't know where I'd even start to change it. If you know where that error is being generated I could look into it but I don't want to break any current functionality.

}

if (!canConfigure(person, portletDefinition.getPortletDefinitionId())) {
logger.warn(
Copy link
Contributor

Choose a reason for hiding this comment

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

With SLF4j loggers you can apply a replace form automatically without appending the properties, like

logger.warn("My text with a prop {} and an other prop {}", prop1, prop2). And you won't have a NPE as it's managed automatically.

@drewwills
Copy link
Contributor

I have some comments I'm planning to add, but I want to start by saying that this is a very desirable & valuable contribution!

@@ -106,3 +106,47 @@ Example response:
}
}
```
### Portlet Preferences

`/api/prefs/{task}`
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest...

/api/v5-6/preferences/{fname} AND /api/v5-6/preferences/{fname}/windowId

The first would be for preferences common to the entire publication record, the second for a specific instance of a publication record.

Copy link
Contributor Author

@j-crawford j-crawford May 30, 2019

Choose a reason for hiding this comment

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

@drewwills Ok, so why the windowId? most webcomponents won't have access to their own windowID since they aren't bundled into uportal and I don't know of an easy way for them to access it- unless you are referring to their layoutID, in the which case they would still need some way of accessing it if we take out the getEntity call. Also they don't need to know their own windowId, uPortal figures all of that from the call made internally anyways. Or are you saying to just put the word windowId to differentiate the two?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@drewwills Also, are you saying that the first would be the definition ones while the second is the entity? I'm not familiar with what you mean by a publication record.


`/api/prefs/{task}`

(example: `http://localhost:8080/uPortal/api/prefs/getprefs/weather` )
Copy link
Contributor

@drewwills drewwills May 20, 2019

Choose a reason for hiding this comment

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

At first glance, it seems we should support GET (read) and PUT (update) out of the gate.

I'm not sure if it makes sense to support POST (create), because a basic collection of favorites is created whenever you publish in the first place.

(REST URIs should not contain verbs within them.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@drewwills I am already supporting gets and puts

}

public PortletPrefsRESTController() {
this.mapper = new ObjectMapper();
Copy link
Contributor

Choose a reason for hiding this comment

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

Can be moved above, to where the member is defined.

* I used this primarily for testing, could be useful given that it returns the windowstate, etc
* up to you if you want to keep it or modify it
* */
@RequestMapping(value = "/prefs/getentity/{fname}", method = RequestMethod.GET)
Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't use verbs in URIs.

I'm not sure this method is a good fit for this @Controller class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@drewwills I wasn't sure that the method was appropriate either, which is why I put the comment above it. Do you want me to remove it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@drewwills note that if you want them to know their own windowId, we will need some sort of replacement call to allow them access to that.

* please set any of these to read-only
*/
// returns the entity composite preferences for the portlet in question
@RequestMapping(value = "/prefs/getprefs/{fname}", method = RequestMethod.GET)
Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't use verbs in URIs.

@ChristianMurphy ChristianMurphy modified the milestones: 5.6.0, 5.7.0 May 23, 2019
@jgribonvald
Copy link
Contributor

@bjagg this is about what we talked on last uPSC ;)

@bjagg
Copy link
Member

bjagg commented Oct 14, 2022

Ah! ... I am finally wrapping up some projects. Starting to get back to coding and hope to review PRs again next week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants