O3-310: allow Put operations on an application-writable config.json file to openmrs/data/frontends/config.json #57

jnsereko · 2023-02-28T11:47:15Z

There should be a config.json file that the application is allowed to manage, which lives in the application data directory.

module-spa should have a REST endpoint PUT /ws/frontend/config that sets the value of this file.

The file is served the usual way along with everything else in the frontend/ directory.

The Implementer Tools should have a button that updates that file with the temporary config.

cc @dkayiwa @hadijahkyampeire

Issue Link: https://issues.openmrs.org/browse/O3-310

Screenshots

Uploading screen recording.mov…

config.video.mov

omod/src/main/java/org/openmrs/module/spa/controllers/TrialConfigController.java

omod/src/main/java/org/openmrs/module/spa/filter/SpaFilter.java

omod/src/main/java/org/openmrs/module/spa/servlet/SpaServlet.java

omod/src/test/java/org/openmrs/module/spa/servlet/SpaServletTest.java

omod/src/test/java/org/openmrs/module/spa/servlet/SpaServletWebTest.java

jnsereko

By default, every request to spa/ transformed with an a retrieve the index.html file.
Ideally, i think, this is the reason the controllers i registered with 70e0cdd were actually not being called (silent).

I tried to call them manually but things just got messed up the more, leaving me with implementing the logic in the doGet and doPut.

I also didn't want to reimplement the existing logic of rendering the index.html logic

cc @dkayiwa @ibacher @hadijahkyampeire @vasharma05 @brandones @denniskigen

ibacher

@jnsereko Thanks for the PR! I think there are some decent ideas here, but some things that need to be addressed before this can be merged in.

I think there are some design questions here that need to be handled as well, specifically around how this is supposed to interact with the implementer tools. What should happen when, having created a configuration, I go back into the implementer tools to configure a different component? What if I decide my configuration is broken and I want to get rid of it?

Finally, why is this defaulting to /openmrs/spa/frontend/config instead of, e.g., /openmrs/spa/config.json`?

omod/src/main/java/org/openmrs/module/spa/servlet/SpaServlet.java

ibacher · 2023-03-01T13:36:11Z

PS The requests for /openmrs/spa get sent to a filter (which will run before any controllers) which forward the request to the servlet, which is why your controllers weren't getting called.

jnsereko · 2023-03-07T00:45:59Z

@ibacher @dkayiwa, i think we have to change the endpoint simply to config.json than the earlier frontend/config.json so as to simplify this requirement
By default the spa module tries to get the url as a file under the frontend directory. So a request like /openmrs/spa/config.json will be resolved as /openmrs/data/frontend/config.json while a request like /openmrs/spa/frontend/config.json will be transformed to /openmrs/data/frontend/frontend/config.json

I have also removed creation of the config directory because it seams useless

jnsereko · 2023-03-08T00:50:52Z

@dkayiwa @ibacher kindly review

omod/src/main/java/org/openmrs/module/spa/SpaConstants.java

jnsereko · 2023-03-09T12:07:07Z

hey @ibacher @dkayiwa, is this fit for merge?

omod/pom.xml

omod/src/main/java/org/openmrs/module/spa/SpaConstants.java

omod/src/main/java/org/openmrs/module/spa/servlet/SpaServlet.java

dkayiwa · 2023-03-26T22:09:51Z

@jnsereko are you still working on this?

jnsereko · 2023-03-27T12:04:51Z

@dkayiwa @ibacher I have cleaned the code

omod/src/main/java/org/openmrs/module/spa/servlet/SpaServlet.java

ibacher · 2023-05-29T12:34:42Z

Is it that 3.x has finally migrated away form using the this module?

The Docker images have never used module-spa.

jnsereko · 2023-05-29T16:14:45Z

The reason why I asked @ibacher is because deployed the spa module into docker for 3.x but all requests to spa/config.json actually returned 404 yet it used to work sometime back (on opening this PR)

…

On Mon, May 29, 2023, 3:34 PM Ian ***@***.***> wrote: Is it that 3.x has finally migrated away form using the this module? The Docker images have never used module-spa. — Reply to this email directly, view it on GitHub <#57 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AN2Q67ZXA5VF4HBKG2MWE4DXISJWZANCNFSM6AAAAAAVKTVR74> . You are receiving this because you were mentioned.Message ID: ***@***.***>

ibacher · 2023-05-29T16:19:26Z

When you were testing this previously, I'm pretty sure you were using the SDK. We haven't changed anything about how the routing works. If you're using the Docker containers, requests to /openmrs/spa will never hit the backend... and that's been the case since we put the Docker containers together. (This is, in fact, what allows us to provide the "single-page" experience; any request to /openmrs/spa is forwarded to the frontend container and any request for a URL in the frontend container that does not otherwise resolve to something serves the single-page content).

jnsereko · 2023-05-29T16:32:54Z

This now makes sense @ibacher Maybe I was using the sdk to test those days but now using docker. Actually logs were showed only frontend and getway images. I was wondering why the backed was not showing. Since I prefer docker, and now that all requests to */spa/* go to frontend, how can I override this? Additionally, would there another better option?

…

On Mon, May 29, 2023, 7:19 PM Ian ***@***.***> wrote: When you were testing this previously, I'm pretty sure you were using the SDK. We haven't changed anything about how the routing works. If you're using the Docker containers, requests to /openmrs/spa will never hit the backend... and that's been the case since we put the Docker containers together. (This is, in fact, what allows us to provide the "single-page" experience; any request to /openmrs/spa is forwarded to the frontend container and any request for a URL in the frontend container that does not otherwise resolve to something serves the single-page content). — Reply to this email directly, view it on GitHub <#57 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AN2Q675TFNAHBUMBCX3ANUTXITEBVANCNFSM6AAAAAAVKTVR74> . You are receiving this because you were mentioned.Message ID: ***@***.***>

jnsereko · 2023-06-01T14:30:40Z

When you were testing this previously, I'm pretty sure you were using the SDK. We haven't changed anything about how the routing works. If you're using the Docker containers, requests to /openmrs/spa will never hit the backend... and that's been the case since we put the Docker containers together. (This is, in fact, what allows us to provide the "single-page" experience; any request to /openmrs/spa is forwarded to the frontend container and any request for a URL in the frontend container that does not otherwise resolve to something serves the single-page content).

i have created a thread for how to move forward because we cant limit this feature to working only in the openmrs-sdk environment.

config.json file name shouldn't be dynamic dynamically serve 'frontend/config.json' endpoint remove unrequited change

jnsereko · 2023-06-12T05:56:36Z

thanks to @dkayiwa, i dynamically defined a servlet to expose an end-point openmrs/ws/frontend/config.json rather than using openmrs/spa/config.json where the request does not reach the backend in the docker environment.

This works fine in both openmrs-sdk and openmrs-docker environment

jnsereko · 2023-06-26T06:09:22Z

@dkayiwa @ibacher the frontend implementation has been approved see openmrs/openmrs-esm-core#629 (review)

dkayiwa · 2023-06-26T07:57:20Z

omod/src/main/java/org/openmrs/module/spa/web/SpaWebComponentRegistrar.java

+    public void setServletContext(ServletContext servletContext) {
+
+        try {
+            ServletRegistration openmrsServletReg = servletContext.getServletRegistration("openmrs");


What is this mapping used for?

I thought we need to ensure that any mapping is supposed to be on top of the OpenMRS servlet. just to eliminate circumstances like https:localhost:port/ws/frontend/config.json

What happens when you remove line 32?

If servletContext.getServletRegistration("openmrs"); isn't necessary @dkayiwa, how can i add mappings to the Openmrs Servlet then?

What happens when you remove line 32?

works fine.

dkayiwa · 2023-06-28T09:11:54Z

omod/src/main/java/org/openmrs/module/spa/servlet/SpaServlet.java

+                String basicAuth = request.getHeader("Authorization");
+                if (basicAuth != null) {
+                    // check that header is in format "Basic ${base64encode(username + ":" + password)}"
+                    if (isValidAuthFormat(response, basicAuth)) return;


https://wiki.openmrs.org/display/docs/Java+Conventions#JavaConventions-CodeStyle

dkayiwa · 2023-06-28T09:14:47Z

omod/src/main/java/org/openmrs/module/spa/web/SpaWebComponentRegistrar.java

+            ServletRegistration servletReg = servletContext.addServlet("spaServlet", new SpaServlet());
+            servletReg.addMapping("/ws/frontend/config.json");
+
+            Dynamic filter = servletContext.addFilter("spaFilter", new SpaFilter());


Is the SpaFilter part of this ticket?

dkayiwa · 2023-06-28T09:25:23Z

omod/src/main/java/org/openmrs/module/spa/servlet/SpaServlet.java

+    @Override
+    protected void doPost(HttpServletRequest request, HttpServletResponse response) throws IOException {
+        String requestURI = request.getRequestURI();
+        if (requestURI.endsWith("/config.json")) {


Can you reduce the deep nesting of these if statements?

brandones · 2023-12-28T10:14:55Z

@jnsereko Do you still have bandwidth to work on this? Could you please work with @dkayiwa to get it across the finish line? It's soooooo close!!

dkayiwa · 2023-12-28T10:18:14Z

I completely agree! 👍

jnsereko · 2023-12-28T10:22:06Z

updating it now

dkayiwa · 2023-12-28T15:24:10Z

Is this functionality going to be only for those who are not using the docker setup of O3 (Because it does not load the spa module)

jnsereko · 2023-12-28T15:36:54Z

Is this functionality going to be only for those who are not using the docker setup of O3 (Because it does not load the spa module)

We can add the module to distro if we choose to. I can do that with your permission

dkayiwa · 2023-12-28T18:22:42Z

I do not think that would be the best option. One other option is adding this to the rest webservices module. This calls for some sort of discussion on talk.

jnsereko · 2023-12-29T05:43:36Z

I do not think that would be the best option. One other option is adding this to the rest webservices module. This calls for some sort of discussion on talk.

I have created a thread: https://talk.openmrs.org/t/adding-openmrs-spa-module-to-dev3-vs-migrating-some-of-its-logic-to-rest-webservices/41250

jnsereko · 2024-01-03T10:18:40Z

@dkayiwa PR created in Rest web services repo openmrs/openmrs-module-webservices.rest#595

jnsereko commented Feb 28, 2023

View reviewed changes

omod/src/main/java/org/openmrs/module/spa/controllers/TrialConfigController.java Outdated Show resolved Hide resolved

jnsereko commented Feb 28, 2023

View reviewed changes

omod/src/main/java/org/openmrs/module/spa/filter/SpaFilter.java Outdated Show resolved Hide resolved

jnsereko commented Feb 28, 2023

View reviewed changes

omod/src/main/java/org/openmrs/module/spa/servlet/SpaServlet.java Outdated Show resolved Hide resolved

jnsereko commented Feb 28, 2023

View reviewed changes

omod/src/test/java/org/openmrs/module/spa/servlet/SpaServletTest.java Outdated Show resolved Hide resolved

jnsereko commented Feb 28, 2023

View reviewed changes

omod/src/test/java/org/openmrs/module/spa/servlet/SpaServletWebTest.java Outdated Show resolved Hide resolved

jnsereko marked this pull request as ready for review March 1, 2023 08:51

jnsereko commented Mar 1, 2023

View reviewed changes

vasharma05 requested review from dkayiwa and ibacher March 1, 2023 12:08

ibacher requested changes Mar 1, 2023

View reviewed changes

jnsereko requested review from ibacher and removed request for dkayiwa March 7, 2023 00:46

jnsereko changed the title ~~O3-310: allow Put operations on an application-writable config.json file to spa/frontends/config~~ O3-310: allow Put operations on an application-writable config.json file to openmrs/data/frontends/config.json Mar 7, 2023

dkayiwa reviewed Mar 8, 2023

View reviewed changes

omod/src/main/java/org/openmrs/module/spa/SpaConstants.java Outdated Show resolved Hide resolved

jnsereko requested review from dkayiwa and removed request for ibacher March 9, 2023 12:06

dkayiwa reviewed Mar 9, 2023

View reviewed changes

omod/pom.xml Outdated Show resolved Hide resolved

dkayiwa reviewed Mar 9, 2023

View reviewed changes

omod/src/main/java/org/openmrs/module/spa/SpaConstants.java Outdated Show resolved Hide resolved

dkayiwa reviewed Mar 9, 2023

View reviewed changes

omod/src/main/java/org/openmrs/module/spa/servlet/SpaServlet.java Outdated Show resolved Hide resolved

dkayiwa reviewed Mar 9, 2023

View reviewed changes

omod/src/main/java/org/openmrs/module/spa/servlet/SpaServlet.java Outdated Show resolved Hide resolved

jnsereko requested a review from dkayiwa March 27, 2023 12:03

jnsereko force-pushed the O3-310 branch from cf3474c to 9e3ca62 Compare March 27, 2023 18:08

dkayiwa reviewed Mar 29, 2023

View reviewed changes

omod/src/main/java/org/openmrs/module/spa/servlet/SpaServlet.java Outdated Show resolved Hide resolved

jnsereko force-pushed the O3-310 branch from 9831ef0 to f7b9748 Compare March 30, 2023 18:33

O3-310: allow Put operations on an application-writable config.json file

6946485

config.json file name shouldn't be dynamic dynamically serve 'frontend/config.json' endpoint remove unrequited change

jnsereko force-pushed the O3-310 branch from 6aafdc3 to 6946485 Compare June 11, 2023 14:50

push to \'/ws/frontend/*\'

20c5af1

dkayiwa reviewed Jun 26, 2023

View reviewed changes

remove unnecessary mappings

6ca7667

jnsereko requested a review from dkayiwa June 28, 2023 04:54

dkayiwa reviewed Jun 28, 2023

View reviewed changes

This was referenced Dec 28, 2023

Revert "feat: O3-310: Add an application-writable config.json file in frontends/" openmrs/openmrs-esm-core#864

Merged

feat: O3-310: Add an application-writable config.json file in frontends/ (attempt #2) openmrs/openmrs-esm-core#865

Open

O3-310: fix violated java conventions

dc87c46

jnsereko requested a review from dkayiwa December 28, 2023 12:09

jnsereko mentioned this pull request Jan 3, 2024

O3-310: allow Put operations on an application-writable config.json file to openmrs/data/frontends/config.json openmrs/openmrs-module-webservices.rest#595

Open

5 tasks

dkayiwa closed this Jan 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

O3-310: allow Put operations on an application-writable config.json file to openmrs/data/frontends/config.json #57

O3-310: allow Put operations on an application-writable config.json file to openmrs/data/frontends/config.json #57

jnsereko commented Feb 28, 2023 •

edited

Loading

jnsereko left a comment

ibacher left a comment

ibacher commented Mar 1, 2023

jnsereko commented Mar 7, 2023 •

edited

Loading

jnsereko commented Mar 8, 2023

jnsereko commented Mar 9, 2023

dkayiwa commented Mar 26, 2023

jnsereko commented Mar 27, 2023

ibacher commented May 29, 2023

jnsereko commented May 29, 2023 via email

ibacher commented May 29, 2023

jnsereko commented May 29, 2023 via email

jnsereko commented Jun 1, 2023 •

edited

Loading

jnsereko commented Jun 12, 2023 •

edited

Loading

jnsereko commented Jun 26, 2023 •

edited

Loading

dkayiwa Jun 26, 2023

jnsereko Jun 26, 2023

dkayiwa Jun 26, 2023

jnsereko Jun 26, 2023 •

edited

Loading

dkayiwa Jun 26, 2023

jnsereko Jun 26, 2023

dkayiwa Jun 28, 2023

dkayiwa Jun 28, 2023

dkayiwa Jun 28, 2023

brandones commented Dec 28, 2023

dkayiwa commented Dec 28, 2023

jnsereko commented Dec 28, 2023 •

edited

Loading

dkayiwa commented Dec 28, 2023

jnsereko commented Dec 28, 2023

dkayiwa commented Dec 28, 2023

jnsereko commented Dec 29, 2023

jnsereko commented Jan 3, 2024

O3-310: allow Put operations on an application-writable config.json file to openmrs/data/frontends/config.json #57

O3-310: allow Put operations on an application-writable config.json file to openmrs/data/frontends/config.json #57

Conversation

jnsereko commented Feb 28, 2023 • edited Loading

Screenshots

jnsereko left a comment

Choose a reason for hiding this comment

ibacher left a comment

Choose a reason for hiding this comment

ibacher commented Mar 1, 2023

jnsereko commented Mar 7, 2023 • edited Loading

jnsereko commented Mar 8, 2023

jnsereko commented Mar 9, 2023

dkayiwa commented Mar 26, 2023

jnsereko commented Mar 27, 2023

ibacher commented May 29, 2023

jnsereko commented May 29, 2023 via email

ibacher commented May 29, 2023

jnsereko commented May 29, 2023 via email

jnsereko commented Jun 1, 2023 • edited Loading

jnsereko commented Jun 12, 2023 • edited Loading

jnsereko commented Jun 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnsereko Jun 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brandones commented Dec 28, 2023

dkayiwa commented Dec 28, 2023

jnsereko commented Dec 28, 2023 • edited Loading

dkayiwa commented Dec 28, 2023

jnsereko commented Dec 28, 2023

dkayiwa commented Dec 28, 2023

jnsereko commented Dec 29, 2023

jnsereko commented Jan 3, 2024

jnsereko commented Feb 28, 2023 •

edited

Loading

jnsereko commented Mar 7, 2023 •

edited

Loading

jnsereko commented Jun 1, 2023 •

edited

Loading

jnsereko commented Jun 12, 2023 •

edited

Loading

jnsereko commented Jun 26, 2023 •

edited

Loading

jnsereko Jun 26, 2023 •

edited

Loading

jnsereko commented Dec 28, 2023 •

edited

Loading